diff options
author | Dmitriy Zaporozhets <dmitriy.zaporozhets@gmail.com> | 2016-08-02 15:38:03 +0300 |
---|---|---|
committer | Dmitriy Zaporozhets <dmitriy.zaporozhets@gmail.com> | 2016-08-02 15:38:03 +0300 |
commit | 6515ec09bbfa25027d1b8a1240e09bc1c6edbcfb (patch) | |
tree | 0113d36deb3a856a4effe6f10f054e6019382a95 | |
parent | f8aeb8cdac98108bca5d1be2a382c32df6a500e5 (diff) | |
download | gitlab-ce-6515ec09bbfa25027d1b8a1240e09bc1c6edbcfb.tar.gz |
Chnage the way how merge request diff is initialized and saved
Signed-off-by: Dmitriy Zaporozhets <dmitriy.zaporozhets@gmail.com>
-rw-r--r-- | app/models/merge_request.rb | 15 | ||||
-rw-r--r-- | app/models/merge_request_diff.rb | 54 | ||||
-rw-r--r-- | app/services/merge_requests/create_service.rb | 1 | ||||
-rw-r--r-- | features/project/merge_requests.feature | 2 | ||||
-rw-r--r-- | features/steps/project/merge_requests.rb | 6 | ||||
-rw-r--r-- | spec/factories/merge_requests.rb | 6 | ||||
-rw-r--r-- | spec/models/merge_request_diff_spec.rb | 12 |
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 |