Merge branch 'dz-improve-mr-compar' into 'master'

Improve merge request versions compare logic

## What does this MR do?

Changes the way how we compare between for merge request versions 

## Are there points in the code the reviewer needs to double check?

no 

## Why was this MR needed?

So when I squash+rebase my commit I can get more accurate diff on what changed between versions

## Screenshots (if relevant)

in discussion (below)

## Does this MR meet the acceptance criteria?

- [x] [CHANGELOG](https://gitlab.com/gitlab-org/gitlab-ce/blob/master/CHANGELOG) entry added
- [ ] ~~[Documentation created/updated](https://gitlab.com/gitlab-org/gitlab-ce/blob/master/doc/development/doc_styleguide.md)~~
- [ ] ~~API support added~~
- Tests
  - [x] Added for this feature/bug
  - [x] All builds are passing
- [x] Conform by the [merge request performance guides](http://docs.gitlab.com/ce/development/merge_request_performance_guidelines.html)
- [x] Conform by the [style guides](https://gitlab.com/gitlab-org/gitlab-ce/blob/master/CONTRIBUTING.md#style-guides)
- [x] Branch has no merge conflicts with `master` (if you do - rebase it please)
- [x] [Squashed related commits together](https://git-scm.com/book/en/Git-Tools-Rewriting-History#Squashing-Commits)

## What are the relevant issue numbers?

https://gitlab.com/gitlab-org/gitlab-ce/issues/22368, https://gitlab.com/gitlab-org/gitlab-ce/issues/22721

See merge request !6589

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

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

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)

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;
   }

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

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

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

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

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')

spec/models/merge_request_diff_spec.rb

@@ -74,6 +74,7 @@ describe MergeRequestDiff, models: true do
         end
       end
     end
+  end
 
   describe '#commits_sha' do
     shared_examples 'returning all commits SHA' do
@@ -96,5 +97,20 @@ describe MergeRequestDiff, models: true 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

spec/services/compare_service_spec.rb

+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