From 5cce1278adb168cd9fa4f189e3656a59726b4e6c Mon Sep 17 00:00:00 2001 From: Douwe Maan Date: Wed, 27 Jan 2016 17:23:59 +0100 Subject: [PATCH 1/2] Correctly determine MR diff base when MR has merge conflicts --- CHANGELOG | 3 +- .../projects/compare_controller.rb | 2 +- app/models/merge_request.rb | 4 +-- app/models/merge_request_diff.rb | 28 ++++++++----------- app/models/project.rb | 4 +++ 5 files changed, 19 insertions(+), 22 deletions(-) diff --git a/CHANGELOG b/CHANGELOG index 4b283f235a4..e7273e017ea 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -20,8 +20,7 @@ v 8.4.2 (unreleased) improvement when checking if a repository was empty - Add instrumentation for Gitlab::Git::Repository instance methods so we can track them in Performance Monitoring. - -v 8.4.2 (unreleased) + - Correctly highlight MR diff when MR has merge conflicts - Fix method undefined when using external commit status in builds v 8.4.1 diff --git a/app/controllers/projects/compare_controller.rb b/app/controllers/projects/compare_controller.rb index f8ec76cd4e5..7bbe75b3974 100644 --- a/app/controllers/projects/compare_controller.rb +++ b/app/controllers/projects/compare_controller.rb @@ -21,7 +21,7 @@ class Projects::CompareController < Projects::ApplicationController @commits = Commit.decorate(compare_result.commits, @project) @diffs = compare_result.diffs @commit = @project.commit(head_ref) - @base_commit = @project.commit(base_ref) + @base_commit = @project.merge_base_commit(base_ref, head_ref) @diff_refs = [@base_commit, @commit] @line_notes = [] end diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb index 41dd248d80a..85dce768cf3 100644 --- a/app/models/merge_request.rb +++ b/app/models/merge_request.rb @@ -184,7 +184,7 @@ class MergeRequest < ActiveRecord::Base if merge_request_diff merge_request_diff.base_commit else - self.target_project.commit(self.target_branch) + self.target_project.merge_base_commit(self.source_sha, self.target_branch) end end @@ -489,7 +489,7 @@ class MergeRequest < ActiveRecord::Base end def source_sha - commits.first.sha + last_commit.sha end def fetch_ref diff --git a/app/models/merge_request_diff.rb b/app/models/merge_request_diff.rb index ba0194cd0a6..c95179d6046 100644 --- a/app/models/merge_request_diff.rb +++ b/app/models/merge_request_diff.rb @@ -48,14 +48,11 @@ class MergeRequestDiff < ActiveRecord::Base end def diffs_no_whitespace - # Get latest sha of branch from source project - source_sha = merge_request.source_project.commit(source_branch).sha - compare_result = Gitlab::CompareResult.new( Gitlab::Git::Compare.new( - merge_request.target_project.repository.raw_repository, - merge_request.target_branch, - source_sha, + self.repository.raw_repository, + self.target_branch, + self.source_sha, ), { ignore_whitespace_change: true } ) @diffs_no_whitespace ||= load_diffs(dump_commits(compare_result.diffs)) @@ -83,8 +80,6 @@ class MergeRequestDiff < ActiveRecord::Base @last_commit_short_sha ||= last_commit.short_id end - private - def dump_commits(commits) commits.map(&:to_hash) end @@ -163,7 +158,7 @@ class MergeRequestDiff < ActiveRecord::Base self.st_diffs = new_diffs - self.base_commit_sha = merge_request.target_project.commit(target_branch).try(:sha) + self.base_commit_sha = self.repository.merge_base(self.source_sha, self.target_branch) self.save end @@ -181,7 +176,10 @@ class MergeRequestDiff < ActiveRecord::Base merge_request.target_project.repository end - private + def source_sha + source_commit = merge_request.source_project.commit(source_branch) + source_commit.try(:sha) + end def compare_result @compare_result ||= @@ -189,15 +187,11 @@ class MergeRequestDiff < ActiveRecord::Base # Update ref for merge request merge_request.fetch_ref - # Get latest sha of branch from source project - source_commit = merge_request.source_project.commit(source_branch) - source_sha = source_commit.try(:sha) - Gitlab::CompareResult.new( Gitlab::Git::Compare.new( - merge_request.target_project.repository.raw_repository, - merge_request.target_branch, - source_sha, + self.repository.raw_repository, + self.target_branch, + self.source_sha ) ) end diff --git a/app/models/project.rb b/app/models/project.rb index 4bd51449c25..488dc98c17f 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -348,6 +348,10 @@ class Project < ActiveRecord::Base repository.commit(id) end + def merge_base_commit(first_commit_id, second_commit_id) + repository.commit(repository.merge_base(first_commit_id, second_commit_id)) + end + def saved? id && persisted? end From 30b0d06e9f74f0068926314ec03b003fbd86c8f2 Mon Sep 17 00:00:00 2001 From: Douwe Maan Date: Thu, 28 Jan 2016 15:10:48 +0100 Subject: [PATCH 2/2] Fix specs --- app/models/merge_request.rb | 4 ++-- app/models/project.rb | 3 ++- app/models/repository.rb | 2 ++ spec/models/build_spec.rb | 2 +- 4 files changed, 7 insertions(+), 4 deletions(-) diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb index 85dce768cf3..0af60645545 100644 --- a/app/models/merge_request.rb +++ b/app/models/merge_request.rb @@ -183,7 +183,7 @@ class MergeRequest < ActiveRecord::Base def diff_base_commit if merge_request_diff merge_request_diff.base_commit - else + elsif source_sha self.target_project.merge_base_commit(self.source_sha, self.target_branch) end end @@ -489,7 +489,7 @@ class MergeRequest < ActiveRecord::Base end def source_sha - last_commit.sha + last_commit.try(:sha) end def fetch_ref diff --git a/app/models/project.rb b/app/models/project.rb index 488dc98c17f..238932f59a7 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -349,7 +349,8 @@ class Project < ActiveRecord::Base end def merge_base_commit(first_commit_id, second_commit_id) - repository.commit(repository.merge_base(first_commit_id, second_commit_id)) + sha = repository.merge_base(first_commit_id, second_commit_id) + repository.commit(sha) if sha end def saved? diff --git a/app/models/repository.rb b/app/models/repository.rb index 6c1ee4b29cd..130daddd9d1 100644 --- a/app/models/repository.rb +++ b/app/models/repository.rb @@ -589,6 +589,8 @@ class Repository def merge_base(first_commit_id, second_commit_id) rugged.merge_base(first_commit_id, second_commit_id) + rescue Rugged::ReferenceError + nil end def is_ancestor?(ancestor_id, descendant_id) diff --git a/spec/models/build_spec.rb b/spec/models/build_spec.rb index d30bc7d0554..606340d87e4 100644 --- a/spec/models/build_spec.rb +++ b/spec/models/build_spec.rb @@ -1,7 +1,7 @@ require 'spec_helper' describe Ci::Build, models: true do - let(:project) { FactoryGirl.create :empty_project } + let(:project) { FactoryGirl.create :project } let(:commit) { FactoryGirl.create :ci_commit, project: project } let(:build) { FactoryGirl.create :ci_build, commit: commit }