diff options
| -rw-r--r-- | CHANGELOG | 1 | ||||
| -rw-r--r-- | Gemfile | 2 | ||||
| -rw-r--r-- | Gemfile.lock | 4 | ||||
| -rw-r--r-- | app/assets/stylesheets/pages/merge_requests.scss | 10 | ||||
| -rw-r--r-- | app/helpers/merge_requests_helper.rb | 4 | ||||
| -rw-r--r-- | app/models/compare.rb | 21 | ||||
| -rw-r--r-- | app/models/merge_request_diff.rb | 7 | ||||
| -rw-r--r-- | app/services/compare_service.rb | 7 | ||||
| -rw-r--r-- | app/views/projects/merge_requests/show/_versions.html.haml | 10 | ||||
| -rw-r--r-- | spec/models/merge_request_diff_spec.rb | 46 | ||||
| -rw-r--r-- | spec/services/compare_service_spec.rb | 21 |
11 files changed, 101 insertions, 32 deletions
diff --git a/CHANGELOG b/CHANGELOG index 3b79f14076a..ab04c55b451 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -130,6 +130,7 @@ v 8.12.4 - Fix failed project deletion when feature visibility set to private. !6688 - Prevent claiming associated model IDs via import. - Set GitLab project exported file permissions to owner only + - Improve the way merge request versions are compared with each other v 8.12.3 - Update Gitlab Shell to support low IO priority for storage moves @@ -51,7 +51,7 @@ gem 'browser', '~> 2.2' # Extracting information from a git repository # Provide access to Gitlab::Git library -gem 'gitlab_git', '~> 10.6.7' +gem 'gitlab_git', '~> 10.6.8' # LDAP Auth # GitLab fork with several improvements to original library. For full list of changes diff --git a/Gemfile.lock b/Gemfile.lock index 924e3a6781a..067908af3fb 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -282,7 +282,7 @@ GEM diff-lcs (~> 1.1) mime-types (>= 1.16, < 3) posix-spawn (~> 0.3) - gitlab_git (10.6.7) + gitlab_git (10.6.8) activesupport (~> 4.0) charlock_holmes (~> 0.7.3) github-linguist (~> 4.7.0) @@ -866,7 +866,7 @@ DEPENDENCIES github-linguist (~> 4.7.0) github-markup (~> 1.4) gitlab-flowdock-git-hook (~> 1.0.1) - gitlab_git (~> 10.6.7) + gitlab_git (~> 10.6.8) gitlab_omniauth-ldap (~> 1.2.1) gollum-lib (~> 4.2) gollum-rugged_adapter (~> 0.4.2) diff --git a/app/assets/stylesheets/pages/merge_requests.scss b/app/assets/stylesheets/pages/merge_requests.scss index 1006b3e62e8..7cf69c56d15 100644 --- a/app/assets/stylesheets/pages/merge_requests.scss +++ b/app/assets/stylesheets/pages/merge_requests.scss @@ -401,8 +401,12 @@ padding: 16px; } + .content-block { + border-top: 1px solid $border-color; + padding: $gl-padding-top $gl-padding; + } + .comments-disabled-notif { - padding: 10px 16px; .btn { margin-left: 5px; } @@ -413,10 +417,6 @@ margin: 0 7px; } - .comments-disabled-notif { - border-top: 1px solid $border-color; - } - .dropdown-title { color: $gl-text-color; } diff --git a/app/helpers/merge_requests_helper.rb b/app/helpers/merge_requests_helper.rb index b0a76765d97..249cb44e9d5 100644 --- a/app/helpers/merge_requests_helper.rb +++ b/app/helpers/merge_requests_helper.rb @@ -123,4 +123,8 @@ module MergeRequestsHelper def version_index(merge_request_diff) @merge_request_diffs.size - @merge_request_diffs.index(merge_request_diff) end + + def different_base?(version1, version2) + version1 && version2 && version1.base_commit_sha != version2.base_commit_sha + end end diff --git a/app/models/compare.rb b/app/models/compare.rb index 4856510f526..3a8bbcb1acd 100644 --- a/app/models/compare.rb +++ b/app/models/compare.rb @@ -11,9 +11,10 @@ class Compare end end - def initialize(compare, project) + def initialize(compare, project, straight: false) @compare = compare @project = project + @straight = straight end def commits @@ -45,6 +46,18 @@ class Compare end end + def start_commit_sha + start_commit.try(:sha) + end + + def base_commit_sha + base_commit.try(:sha) + end + + def head_commit_sha + commit.try(:sha) + end + def raw_diffs(*args) @compare.diffs(*args) end @@ -58,9 +71,9 @@ class Compare def diff_refs Gitlab::Diff::DiffRefs.new( - base_sha: base_commit.try(:sha), - start_sha: start_commit.try(:sha), - head_sha: commit.try(:sha) + base_sha: @straight ? start_commit_sha : base_commit_sha, + start_sha: start_commit_sha, + head_sha: head_commit_sha ) end end diff --git a/app/models/merge_request_diff.rb b/app/models/merge_request_diff.rb index 3f7e96186a1..b8a10b7968e 100644 --- a/app/models/merge_request_diff.rb +++ b/app/models/merge_request_diff.rb @@ -167,8 +167,11 @@ class MergeRequestDiff < ActiveRecord::Base self == merge_request.merge_request_diff end - def compare_with(sha) - CompareService.new.execute(project, head_commit_sha, project, sha) + def compare_with(sha, straight: true) + # When compare merge request versions we want diff A..B instead of A...B + # so we handle cases when user does squash and rebase of the commits between versions. + # For this reason we set straight to true by default. + CompareService.new.execute(project, head_commit_sha, project, sha, straight: straight) end private diff --git a/app/services/compare_service.rb b/app/services/compare_service.rb index 6d6075628af..5e8fafca98c 100644 --- a/app/services/compare_service.rb +++ b/app/services/compare_service.rb @@ -3,7 +3,7 @@ require 'securerandom' # Compare 2 branches for one repo or between repositories # and return Gitlab::Git::Compare object that responds to commits and diffs class CompareService - def execute(source_project, source_branch, target_project, target_branch) + def execute(source_project, source_branch, target_project, target_branch, straight: false) source_commit = source_project.commit(source_branch) return unless source_commit @@ -23,9 +23,10 @@ class CompareService raw_compare = Gitlab::Git::Compare.new( target_project.repository.raw_repository, target_branch, - source_sha + source_sha, + straight ) - Compare.new(raw_compare, target_project) + Compare.new(raw_compare, target_project, straight: straight) end end diff --git a/app/views/projects/merge_requests/show/_versions.html.haml b/app/views/projects/merge_requests/show/_versions.html.haml index 988ac0feae1..eab48b78cb3 100644 --- a/app/views/projects/merge_requests/show/_versions.html.haml +++ b/app/views/projects/merge_requests/show/_versions.html.haml @@ -64,6 +64,16 @@ #{@merge_request.target_branch} (base) .monospace #{short_sha(@merge_request_diff.base_commit_sha)} + - if different_base?(@start_version, @merge_request_diff) + .content-block + = icon('info-circle') + Selected versions have different base commits. + Changes will include + = link_to namespace_project_compare_path(@project.namespace, @project, from: @start_version.base_commit_sha, to: @merge_request_diff.base_commit_sha) do + new commits + from + %code #{@merge_request.target_branch} + - unless @merge_request_diff.latest? && !@start_sha .comments-disabled-notif.content-block = icon('info-circle') diff --git a/spec/models/merge_request_diff_spec.rb b/spec/models/merge_request_diff_spec.rb index f27de0948ee..e5007424041 100644 --- a/spec/models/merge_request_diff_spec.rb +++ b/spec/models/merge_request_diff_spec.rb @@ -74,27 +74,43 @@ describe MergeRequestDiff, models: true do end end end + end - describe '#commits_sha' do - shared_examples 'returning all commits SHA' do - it 'returns all commits SHA' do - commits_sha = subject.commits_sha + describe '#commits_sha' do + shared_examples 'returning all commits SHA' do + it 'returns all commits SHA' do + commits_sha = subject.commits_sha - expect(commits_sha).to eq(subject.commits.map(&:sha)) - end + expect(commits_sha).to eq(subject.commits.map(&:sha)) end + end - context 'when commits were loaded' do - before do - subject.commits - end - - it_behaves_like 'returning all commits SHA' + context 'when commits were loaded' do + before do + subject.commits end - context 'when commits were not loaded' do - it_behaves_like 'returning all commits SHA' - end + it_behaves_like 'returning all commits SHA' + end + + context 'when commits were not loaded' do + it_behaves_like 'returning all commits SHA' + end + end + + describe '#compare_with' do + subject { create(:merge_request, source_branch: 'fix').merge_request_diff } + + it 'delegates compare to the service' do + expect(CompareService).to receive(:new).and_call_original + + subject.compare_with(nil) + end + + it 'uses git diff A..B approach by default' do + diffs = subject.compare_with('0b4bc9a49b562e85de7cc9e834518ea6828729b9').diffs + + expect(diffs.size).to eq(3) end end end diff --git a/spec/services/compare_service_spec.rb b/spec/services/compare_service_spec.rb new file mode 100644 index 00000000000..3760f19aaa2 --- /dev/null +++ b/spec/services/compare_service_spec.rb @@ -0,0 +1,21 @@ +require 'spec_helper' + +describe CompareService, services: true do + let(:project) { create(:project) } + let(:user) { create(:user) } + let(:service) { described_class.new } + + describe '#execute' do + context 'compare with base, like feature...fix' do + subject { service.execute(project, 'feature', project, 'fix', straight: false) } + + it { expect(subject.diffs.size).to eq(1) } + end + + context 'straight compare, like feature..fix' do + subject { service.execute(project, 'feature', project, 'fix', straight: true) } + + it { expect(subject.diffs.size).to eq(3) } + end + end +end |