From c143003bfbd5cda725451c38ff1eca8ba469409b Mon Sep 17 00:00:00 2001 From: Dmitriy Zaporozhets Date: Thu, 29 Sep 2016 13:53:18 +0300 Subject: [PATCH 01/10] Bump gitlab_git to 10.6.8 Signed-off-by: Dmitriy Zaporozhets --- Gemfile | 2 +- Gemfile.lock | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/Gemfile b/Gemfile index c5f1ce26daf..59ff78c8fae 100644 --- a/Gemfile +++ b/Gemfile @@ -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 2feec4c4eb5..edccf870821 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -280,7 +280,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) @@ -863,7 +863,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) From ac4db38094f4a68a81b0a7570c5835f663c01cfd Mon Sep 17 00:00:00 2001 From: Dmitriy Zaporozhets Date: Thu, 29 Sep 2016 14:04:50 +0300 Subject: [PATCH 02/10] Use straight diff approach when compare merge request versions Signed-off-by: Dmitriy Zaporozhets --- app/models/compare.rb | 5 ++++- app/models/merge_request_diff.rb | 7 +++++-- app/services/compare_service.rb | 7 ++++--- 3 files changed, 13 insertions(+), 6 deletions(-) diff --git a/app/models/compare.rb b/app/models/compare.rb index 4856510f526..4b568a1d11c 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 @@ -36,6 +37,8 @@ class Compare alias_method :commit, :head_commit def base_commit + return start_commit if @straight + return @base_commit if defined?(@base_commit) @base_commit = if start_commit && head_commit diff --git a/app/models/merge_request_diff.rb b/app/models/merge_request_diff.rb index 3f7e96186a1..0ea9e892be2 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 squash and rebase commits in one of versions. + # For this reason we set straight to true by default. + CompareService.new.execute(project, head_commit_sha, project, sha, straight) end private diff --git a/app/services/compare_service.rb b/app/services/compare_service.rb index 6d6075628af..6df3b958b8a 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) end end From 1bb64ec99476451d67463e3508b449aeea7e694f Mon Sep 17 00:00:00 2001 From: Dmitriy Zaporozhets Date: Thu, 29 Sep 2016 17:21:19 +0300 Subject: [PATCH 03/10] Add spec for compare service Signed-off-by: Dmitriy Zaporozhets --- spec/services/compare_service_spec.rb | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+) create mode 100644 spec/services/compare_service_spec.rb diff --git a/spec/services/compare_service_spec.rb b/spec/services/compare_service_spec.rb new file mode 100644 index 00000000000..d809f291b44 --- /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', false) } + + it { expect(subject.diffs.size).to eq(1) } + end + + context 'straight compare, like feature..fix' do + subject { service.execute(project, 'feature', project, 'fix', true) } + + it { expect(subject.diffs.size).to eq(3) } + end + end +end From afe28b7be3a3a5a9db6e5a32465c0e3a8e9cb83d Mon Sep 17 00:00:00 2001 From: Dmitriy Zaporozhets Date: Thu, 29 Sep 2016 17:23:36 +0300 Subject: [PATCH 04/10] Add mr version improvement to changelog Signed-off-by: Dmitriy Zaporozhets --- CHANGELOG | 3 +++ 1 file changed, 3 insertions(+) diff --git a/CHANGELOG b/CHANGELOG index e478e8c3365..51870619fb4 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -127,6 +127,9 @@ 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.4 (unreleased) v 8.12.3 - Update Gitlab Shell to support low IO priority for storage moves From cdcc11d48fe6d04f6a1cfebc39bcea71d35b3e4a Mon Sep 17 00:00:00 2001 From: Dmitriy Zaporozhets Date: Thu, 29 Sep 2016 17:46:47 +0300 Subject: [PATCH 05/10] Improve tests for merge request diff model Signed-off-by: Dmitriy Zaporozhets --- app/models/merge_request_diff.rb | 2 +- spec/models/merge_request_diff_spec.rb | 40 ++++++++++++++++---------- 2 files changed, 26 insertions(+), 16 deletions(-) diff --git a/app/models/merge_request_diff.rb b/app/models/merge_request_diff.rb index 0ea9e892be2..6ad2201573d 100644 --- a/app/models/merge_request_diff.rb +++ b/app/models/merge_request_diff.rb @@ -169,7 +169,7 @@ class MergeRequestDiff < ActiveRecord::Base 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 squash and rebase commits in one of versions. + # 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) end diff --git a/spec/models/merge_request_diff_spec.rb b/spec/models/merge_request_diff_spec.rb index f27de0948ee..69b8afd22a3 100644 --- a/spec/models/merge_request_diff_spec.rb +++ b/spec/models/merge_request_diff_spec.rb @@ -74,27 +74,37 @@ 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 - context 'when commits were loaded' do - before do - subject.commits - end + 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 - context 'when commits were not loaded' do - it_behaves_like 'returning all commits SHA' - end + describe '#compare_with' do + subject { create(:merge_request).merge_request_diff } + + it 'delegates compare to the service' do + expect(CompareService).to receive(:new).and_call_original + + subject.compare_with('ae73cb07c9eeaf35924a10f713b364d32b2dd34f') end end end From 7421c08ace8a129b37c4942eb21a0b5cf1a73a53 Mon Sep 17 00:00:00 2001 From: Dmitriy Zaporozhets Date: Thu, 29 Sep 2016 18:02:06 +0300 Subject: [PATCH 06/10] Better tests for MergeRequestDiff#compare_with method Signed-off-by: Dmitriy Zaporozhets --- spec/models/merge_request_diff_spec.rb | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/spec/models/merge_request_diff_spec.rb b/spec/models/merge_request_diff_spec.rb index 69b8afd22a3..e5007424041 100644 --- a/spec/models/merge_request_diff_spec.rb +++ b/spec/models/merge_request_diff_spec.rb @@ -99,12 +99,18 @@ describe MergeRequestDiff, models: true do end describe '#compare_with' do - subject { create(:merge_request).merge_request_diff } + 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('ae73cb07c9eeaf35924a10f713b364d32b2dd34f') + 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 From b48c4b2662e7db9d68052392fb34dd2b27d12cf5 Mon Sep 17 00:00:00 2001 From: Dmitriy Zaporozhets Date: Wed, 12 Oct 2016 13:27:59 +0300 Subject: [PATCH 07/10] Refactor straight compare diff code Signed-off-by: Dmitriy Zaporozhets --- app/models/compare.rb | 22 ++++++++++++++++------ app/models/merge_request_diff.rb | 4 ++-- app/services/compare_service.rb | 4 ++-- spec/services/compare_service_spec.rb | 4 ++-- 4 files changed, 22 insertions(+), 12 deletions(-) diff --git a/app/models/compare.rb b/app/models/compare.rb index 4b568a1d11c..3a8bbcb1acd 100644 --- a/app/models/compare.rb +++ b/app/models/compare.rb @@ -11,7 +11,7 @@ class Compare end end - def initialize(compare, project, straight = false) + def initialize(compare, project, straight: false) @compare = compare @project = project @straight = straight @@ -37,8 +37,6 @@ class Compare alias_method :commit, :head_commit def base_commit - return start_commit if @straight - return @base_commit if defined?(@base_commit) @base_commit = if start_commit && head_commit @@ -48,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 @@ -61,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 6ad2201573d..b8a10b7968e 100644 --- a/app/models/merge_request_diff.rb +++ b/app/models/merge_request_diff.rb @@ -167,11 +167,11 @@ class MergeRequestDiff < ActiveRecord::Base self == merge_request.merge_request_diff end - def compare_with(sha, straight = true) + 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) + 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 6df3b958b8a..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, straight = false) + def execute(source_project, source_branch, target_project, target_branch, straight: false) source_commit = source_project.commit(source_branch) return unless source_commit @@ -27,6 +27,6 @@ class CompareService straight ) - Compare.new(raw_compare, target_project, straight) + Compare.new(raw_compare, target_project, straight: straight) end end diff --git a/spec/services/compare_service_spec.rb b/spec/services/compare_service_spec.rb index d809f291b44..3760f19aaa2 100644 --- a/spec/services/compare_service_spec.rb +++ b/spec/services/compare_service_spec.rb @@ -7,13 +7,13 @@ describe CompareService, services: true do describe '#execute' do context 'compare with base, like feature...fix' do - subject { service.execute(project, 'feature', project, 'fix', false) } + 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', true) } + subject { service.execute(project, 'feature', project, 'fix', straight: true) } it { expect(subject.diffs.size).to eq(3) } end From c65c800f8e466ffcfb58613fc775c4afa11f91e6 Mon Sep 17 00:00:00 2001 From: Dmitriy Zaporozhets Date: Wed, 12 Oct 2016 15:09:34 +0300 Subject: [PATCH 08/10] Inform user when comparing 2 version with different base Signed-off-by: Dmitriy Zaporozhets --- app/assets/stylesheets/pages/merge_requests.scss | 11 ++++++----- .../projects/merge_requests/show/_versions.html.haml | 7 +++++++ 2 files changed, 13 insertions(+), 5 deletions(-) diff --git a/app/assets/stylesheets/pages/merge_requests.scss b/app/assets/stylesheets/pages/merge_requests.scss index 1006b3e62e8..c712d6e36b1 100644 --- a/app/assets/stylesheets/pages/merge_requests.scss +++ b/app/assets/stylesheets/pages/merge_requests.scss @@ -401,8 +401,13 @@ padding: 16px; } + .content-block { + padding: $gl-padding-top $gl-padding; + } + .comments-disabled-notif { - padding: 10px 16px; + border-top: 1px solid $border-color; + .btn { margin-left: 5px; } @@ -413,10 +418,6 @@ margin: 0 7px; } - .comments-disabled-notif { - border-top: 1px solid $border-color; - } - .dropdown-title { color: $gl-text-color; } diff --git a/app/views/projects/merge_requests/show/_versions.html.haml b/app/views/projects/merge_requests/show/_versions.html.haml index 988ac0feae1..c282e3868cc 100644 --- a/app/views/projects/merge_requests/show/_versions.html.haml +++ b/app/views/projects/merge_requests/show/_versions.html.haml @@ -64,6 +64,13 @@ #{@merge_request.target_branch} (base) .monospace #{short_sha(@merge_request_diff.base_commit_sha)} + - if @start_version && @start_version.base_commit_sha != @merge_request_diff.base_commit_sha + .content-block + 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 #{@merge_request.target_branch} - unless @merge_request_diff.latest? && !@start_sha .comments-disabled-notif.content-block = icon('info-circle') From d0fb1835158467ce58c59f489384b13ad0f0ff7c Mon Sep 17 00:00:00 2001 From: Dmitriy Zaporozhets Date: Wed, 12 Oct 2016 15:45:14 +0300 Subject: [PATCH 09/10] Fix CHANGELOG Signed-off-by: Dmitriy Zaporozhets --- CHANGELOG | 2 -- 1 file changed, 2 deletions(-) diff --git a/CHANGELOG b/CHANGELOG index 51870619fb4..445566db95f 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -129,8 +129,6 @@ v 8.12.4 - Set GitLab project exported file permissions to owner only - Improve the way merge request versions are compared with each other -v 8.12.4 (unreleased) - v 8.12.3 - Update Gitlab Shell to support low IO priority for storage moves From 3dd7ad5064afa49aa74a7d40ba02fff7e67846d2 Mon Sep 17 00:00:00 2001 From: Dmitriy Zaporozhets Date: Wed, 12 Oct 2016 17:42:32 +0300 Subject: [PATCH 10/10] Improve mr compare message when base is different Signed-off-by: Dmitriy Zaporozhets --- app/assets/stylesheets/pages/merge_requests.scss | 3 +-- app/helpers/merge_requests_helper.rb | 4 ++++ app/views/projects/merge_requests/show/_versions.html.haml | 7 +++++-- 3 files changed, 10 insertions(+), 4 deletions(-) diff --git a/app/assets/stylesheets/pages/merge_requests.scss b/app/assets/stylesheets/pages/merge_requests.scss index c712d6e36b1..7cf69c56d15 100644 --- a/app/assets/stylesheets/pages/merge_requests.scss +++ b/app/assets/stylesheets/pages/merge_requests.scss @@ -402,12 +402,11 @@ } .content-block { + border-top: 1px solid $border-color; padding: $gl-padding-top $gl-padding; } .comments-disabled-notif { - border-top: 1px solid $border-color; - .btn { margin-left: 5px; } 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/views/projects/merge_requests/show/_versions.html.haml b/app/views/projects/merge_requests/show/_versions.html.haml index c282e3868cc..eab48b78cb3 100644 --- a/app/views/projects/merge_requests/show/_versions.html.haml +++ b/app/views/projects/merge_requests/show/_versions.html.haml @@ -64,13 +64,16 @@ #{@merge_request.target_branch} (base) .monospace #{short_sha(@merge_request_diff.base_commit_sha)} - - if @start_version && @start_version.base_commit_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 #{@merge_request.target_branch} + from + %code #{@merge_request.target_branch} + - unless @merge_request_diff.latest? && !@start_sha .comments-disabled-notif.content-block = icon('info-circle')