Chnage the way how merge request diff is initialized and saved

Signed-off-by: Dmitriy Zaporozhets <dmitriy.zaporozhets@gmail.com>

7 files changed, 44 insertions, 52 deletions

diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb
index 85048f928dc..6f2216ab5db 100644
--- a/app/models/merge_request.rb
+++ b/app/models/merge_request.rb
@@ -11,11 +11,13 @@ class MergeRequest < ActiveRecord::Base
   belongs_to :merge_user, class_name: "User"
 
   has_many :merge_request_diffs, dependent: :destroy
+  has_one :merge_request_diff
   has_many :events, as: :target, dependent: :destroy
 
   serialize :merge_params, Hash
 
-  after_update :update_merge_request_diff
+  after_create :ensure_merge_request_diff, unless: :importing?
+  after_update :reload_diff_if_branch_changed
 
   delegate :commits, :real_size, to: :merge_request_diff, prefix: nil
 
@@ -94,7 +96,6 @@ class MergeRequest < ActiveRecord::Base
   validates :merge_user, presence: true, if: :merge_when_build_succeeds?
   validate :validate_branches, unless: [:allow_broken, :importing?]
   validate :validate_fork
-  validates_associated :merge_request_diff, on: :create, unless: [:allow_broken, :importing?]
 
   scope :by_branch, ->(branch_name) { where("(source_branch LIKE :branch) OR (target_branch LIKE :branch)", branch: branch_name) }
   scope :cared, ->(user) { where('assignee_id = :user OR author_id = :user', user: user.id) }
@@ -282,11 +283,11 @@ class MergeRequest < ActiveRecord::Base
     end
   end
 
-  def create_merge_request_diff
-    merge_request_diffs.create
+  def ensure_merge_request_diff
+    merge_request_diff || create_merge_request_diff
   end
 
-  def update_merge_request_diff
+  def reload_diff_if_branch_changed
     if source_branch_changed? || target_branch_changed?
       reload_diff
     end
@@ -689,8 +690,4 @@ class MergeRequest < ActiveRecord::Base
   def keep_around_commit
     project.repository.keep_around(self.merge_commit_sha)
   end
-
-  def merge_request_diff
-    merge_request_diffs.first
-  end
 end
diff --git a/app/models/merge_request_diff.rb b/app/models/merge_request_diff.rb
index b668b62cdac..074d8f5d40a 100644
--- a/app/models/merge_request_diff.rb
+++ b/app/models/merge_request_diff.rb
@@ -22,28 +22,28 @@ class MergeRequestDiff < ActiveRecord::Base
   serialize :st_commits
   serialize :st_diffs
 
-  validates :start_commit_sha, presence: true, unless: :importing?
-  validates :head_commit_sha,  presence: true, unless: :importing?
+  # For compatibility with old MergeRequestDiff which
+  # does not store those variables in database
+  after_initialize :ensure_commits_sha, if: :persisted?
 
-  after_initialize :initialize_commits_sha, unless: :importing?
+  # All diff information is collected from repository after object is created.
+  # It allows you to override variables like head_commit_sha before getting diff.
   after_create :save_git_content, unless: :importing?
-  after_save :keep_around_commits, unless: :importing?
-
-  # Those variables are used for collecting commits and diff from git repository.
-  # After object is created those sha are stored in the database.
-  # However some old MergeRequestDiff records don't
-  # have those variables in the database so we try to initialize it
-  def initialize_commits_sha
-    self.start_commit_sha ||= merge_request.target_branch_sha
-    self.head_commit_sha  ||= persisted? ? last_commit.sha : merge_request.source_branch_sha
-    self.base_commit_sha  ||= find_base_sha
-  end
 
   # Collect information about commits and diff from repository
   # and save it to the database as serialized data
   def save_git_content
+    ensure_commits_sha
     save_commits
+    reload_commits
     save_diffs
+    keep_around_commits
+  end
+
+  def ensure_commits_sha
+    self.start_commit_sha ||= merge_request.target_branch_sha
+    self.head_commit_sha  ||= last_commit.try(:sha) || merge_request.source_branch_sha
+    self.base_commit_sha  ||= find_base_sha
   end
 
   def size
@@ -52,14 +52,15 @@ class MergeRequestDiff < ActiveRecord::Base
 
   def diffs(options={})
     if options[:ignore_whitespace_change]
-      @diffs_no_whitespace ||= begin
-        compare = Gitlab::Git::Compare.new(
-          repository.raw_repository,
-          start_commit_sha,
-          head_commit_sha
-        )
-        compare.diffs(options)
-      end
+      @diffs_no_whitespace ||=
+        begin
+          compare = Gitlab::Git::Compare.new(
+            repository.raw_repository,
+            start_commit_sha,
+            head_commit_sha
+          )
+          compare.diffs(options)
+        end
     else
       @diffs ||= {}
       @diffs[options] ||= load_diffs(st_diffs, options)
@@ -70,6 +71,11 @@ class MergeRequestDiff < ActiveRecord::Base
     @commits ||= load_commits(st_commits || [])
   end
 
+  def reload_commits
+    @commits = nil
+    commits
+  end
+
   def last_commit
     commits.first
   end
@@ -192,7 +198,6 @@ class MergeRequestDiff < ActiveRecord::Base
 
     new_attributes[:st_diffs] = new_diffs
     update_columns_serialized(new_attributes)
-    keep_around_commits
   end
 
   def project
@@ -207,6 +212,9 @@ class MergeRequestDiff < ActiveRecord::Base
     return unless head_commit_sha && start_commit_sha
 
     project.merge_base_commit(head_commit_sha, start_commit_sha).try(:sha)
+  rescue Rugged::OdbError
+    # In case head or start commit does not exist in the repository any more
+    nil
   end
 
   def utf8_st_diffs
diff --git a/app/services/merge_requests/create_service.rb b/app/services/merge_requests/create_service.rb
index 553d7443c86..96a25330af1 100644
--- a/app/services/merge_requests/create_service.rb
+++ b/app/services/merge_requests/create_service.rb
@@ -16,7 +16,6 @@ module MergeRequests
       merge_request.target_project ||= source_project
       merge_request.author = current_user
       merge_request.merge_params['force_remove_source_branch'] = force_remove_source_branch
-      merge_request.merge_request_diffs.build
 
       if merge_request.save
         merge_request.update_attributes(label_ids: label_params)
diff --git a/features/project/merge_requests.feature b/features/project/merge_requests.feature
index 21768c15c17..7673b4284ef 100644
--- a/features/project/merge_requests.feature
+++ b/features/project/merge_requests.feature
@@ -24,7 +24,7 @@ Feature: Project Merge Requests
   Scenario: I should see target branch when it is different from default
     Given project "Shop" have "Bug NS-06" open merge request
     When I visit project "Shop" merge requests page
-    Then I should see "other_branch" branch
+    Then I should see "feature_conflict" branch
 
   Scenario: I should not see the numbers of diverged commits if the branch is rebased on the target
     Given project "Shop" have "Bug NS-07" open merge request with rebased branch
diff --git a/features/steps/project/merge_requests.rb b/features/steps/project/merge_requests.rb
index da848afd48e..74e162aadee 100644
--- a/features/steps/project/merge_requests.rb
+++ b/features/steps/project/merge_requests.rb
@@ -56,8 +56,8 @@ class Spinach::Features::ProjectMergeRequests < Spinach::FeatureSteps
     expect(find('.merge-request-info')).not_to have_content "master"
   end
 
-  step 'I should see "other_branch" branch' do
-    expect(page).to have_content "other_branch"
+  step 'I should see "feature_conflict" branch' do
+    expect(page).to have_content "feature_conflict"
   end
 
   step 'I should see "Bug NS-04" in merge requests' do
@@ -122,7 +122,7 @@ class Spinach::Features::ProjectMergeRequests < Spinach::FeatureSteps
            source_project: project,
            target_project: project,
            source_branch: 'fix',
-           target_branch: 'other_branch',
+           target_branch: 'feature_conflict',
            author: project.users.first,
            description: "# Description header"
           )
diff --git a/spec/factories/merge_requests.rb b/spec/factories/merge_requests.rb
index 05851c49497..c6a08d78b78 100644
--- a/spec/factories/merge_requests.rb
+++ b/spec/factories/merge_requests.rb
@@ -68,11 +68,5 @@ FactoryGirl.define do
     factory :closed_merge_request, traits: [:closed]
     factory :reopened_merge_request, traits: [:reopened]
     factory :merge_request_with_diffs, traits: [:with_diffs]
-
-    after :create do |merge_request|
-      unless merge_request.merge_request_diff
-        merge_request.create_merge_request_diff
-      end
-    end
   end
 end
diff --git a/spec/models/merge_request_diff_spec.rb b/spec/models/merge_request_diff_spec.rb
index 0a55515e8c6..f2954a39b08 100644
--- a/spec/models/merge_request_diff_spec.rb
+++ b/spec/models/merge_request_diff_spec.rb
@@ -1,15 +1,6 @@
 require 'spec_helper'
 
 describe MergeRequestDiff, models: true do
-  describe 'initialize new object' do
-    subject { build(:merge_request).merge_request_diffs.build }
-
-    it { expect(subject).to be_valid }
-    it { expect(subject.head_commit_sha).to eq('5937ac0a7beb003549fc5fd26fc247adbce4a52e') }
-    it { expect(subject.base_commit_sha).to eq('ae73cb07c9eeaf35924a10f713b364d32b2dd34f') }
-    it { expect(subject.start_commit_sha).to eq('0b4bc9a49b562e85de7cc9e834518ea6828729b9') }
-  end
-
   describe 'create new record' do
     subject { create(:merge_request).merge_request_diff }
 
@@ -17,6 +8,9 @@ describe MergeRequestDiff, models: true do
     it { expect(subject).to be_persisted }
     it { expect(subject.commits.count).to eq(5) }
     it { expect(subject.diffs.count).to eq(8) }
+    it { expect(subject.head_commit_sha).to eq('5937ac0a7beb003549fc5fd26fc247adbce4a52e') }
+    it { expect(subject.base_commit_sha).to eq('ae73cb07c9eeaf35924a10f713b364d32b2dd34f') }
+    it { expect(subject.start_commit_sha).to eq('0b4bc9a49b562e85de7cc9e834518ea6828729b9') }
   end
 
   describe '#diffs' do