Merge branch 'dz-merge-request-version'

CHANGELOG

@@ -27,6 +27,9 @@ v 8.11.1 (unreleased)
   - Change using size to use count and caching it for number of group members
 
 v 8.11.0
+  - Add merge request versions !5467
+
+v 8.11.0 (unreleased)
   - Use test coverage value from the latest successful pipeline in badge. !5862
   - Add test coverage report badge. !5708
   - Remove the http_parser.rb dependency by removing the tinder gem. !5758 (tbalthazar)

app/assets/stylesheets/framework/dropdowns.scss

@@ -17,6 +17,12 @@
 
 .dropdown {
   position: relative;
+
+  .btn-link {
+    &:hover {
+      cursor: pointer;
+    }
+  }
 }
 
 .open {

app/assets/stylesheets/pages/merge_requests.scss

@@ -375,6 +375,16 @@
   }
 }
 
+.mr-version-switch {
+  background: $background-color;
+  padding: $gl-btn-padding;
+  color: $gl-placeholder-color;
+
+  a.btn-link {
+    color: $gl-dark-link-color;
+  }
+}
+
 .merge-request-details {
 
   .title {

app/controllers/projects/merge_requests_controller.rb

@@ -83,12 +83,22 @@ class Projects::MergeRequestsController < Projects::ApplicationController
   def diffs
     apply_diff_view_cookie!
 
-    @merge_request_diff = @merge_request.merge_request_diff
+    @merge_request_diff =
+      if params[:diff_id]
+        @merge_request.merge_request_diffs.find(params[:diff_id])
+      else
+        @merge_request.merge_request_diff
+      end
 
     respond_to do |format|
       format.html { define_discussion_vars }
       format.json do
-        @diffs = @merge_request.diffs(diff_options)
+        unless @merge_request_diff.latest?
+          # Disable comments if browsing older version of the diff
+          @diff_notes_disabled = true
+        end
+
+        @diffs = @merge_request_diff.diffs(diff_options)
 
         render json: { html: view_to_html_string("projects/merge_requests/show/_diffs") }
       end

app/models/merge_request.rb

@@ -10,14 +10,16 @@ class MergeRequest < ActiveRecord::Base
   belongs_to :source_project, foreign_key: :source_project_id, class_name: "Project"
   belongs_to :merge_user, class_name: "User"
 
-  has_one :merge_request_diff, dependent: :destroy
+  has_many :merge_request_diffs, dependent: :destroy
+  has_one :merge_request_diff,
+    -> { order('merge_request_diffs.id DESC') }
 
   has_many :events, as: :target, dependent: :destroy
 
   serialize :merge_params, Hash
 
-  after_create :create_merge_request_diff, unless: :importing?
-  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
 
@@ -170,10 +172,10 @@ class MergeRequest < ActiveRecord::Base
   end
 
   def diffs(diff_options = nil)
-    if self.compare
-      self.compare.diffs(diff_options)
+    if compare
+      compare.diffs(diff_options)
     else
-      Gitlab::Diff::FileCollection::MergeRequest.new(self, diff_options: diff_options)
+      merge_request_diff.diffs(diff_options)
     end
   end
 
@@ -184,8 +186,8 @@ class MergeRequest < ActiveRecord::Base
   def diff_base_commit
     if persisted?
       merge_request_diff.base_commit
-    elsif diff_start_commit && diff_head_commit
-      self.target_project.merge_base_commit(diff_start_sha, diff_head_sha)
+    else
+      branch_merge_base_commit
     end
   end
 
@@ -246,6 +248,15 @@ class MergeRequest < ActiveRecord::Base
     target_project.repository.commit(target_branch) if target_branch_ref
   end
 
+  def branch_merge_base_commit
+    start_sha = target_branch_sha
+    head_sha  = source_branch_sha
+
+    if start_sha && head_sha
+      target_project.merge_base_commit(start_sha, head_sha)
+    end
+  end
+
   def target_branch_sha
     @target_branch_sha || target_branch_head.try(:sha)
   end
@@ -267,16 +278,16 @@ class MergeRequest < ActiveRecord::Base
   # Return diff_refs instance trying to not touch the git repository
   def diff_sha_refs
     if merge_request_diff && merge_request_diff.diff_refs_by_sha?
-      return Gitlab::Diff::DiffRefs.new(
-        base_sha:  merge_request_diff.base_commit_sha,
-        start_sha: merge_request_diff.start_commit_sha,
-        head_sha:  merge_request_diff.head_commit_sha
-      )
+      merge_request_diff.diff_refs
     else
       diff_refs
     end
   end
 
+  def branch_merge_base_sha
+    branch_merge_base_commit.try(:sha)
+  end
+
   def validate_branches
     if target_project == source_project && target_branch == source_branch
       errors.add :branch_conflict, "You can not use same project/branch for source and target"
@@ -309,21 +320,31 @@ class MergeRequest < ActiveRecord::Base
     end
   end
 
-  def update_merge_request_diff
+  def ensure_merge_request_diff
+    merge_request_diff || create_merge_request_diff
+  end
+
+  def create_merge_request_diff
+    merge_request_diffs.create
+    reload_merge_request_diff
+  end
+
+  def reload_merge_request_diff
+    merge_request_diff(true)
+  end
+
+  def reload_diff_if_branch_changed
     if source_branch_changed? || target_branch_changed?
       reload_diff
     end
   end
 
   def reload_diff
-    return unless merge_request_diff && open?
+    return unless open?
 
     old_diff_refs = self.diff_refs
-
-    merge_request_diff.reload_content
-
+    create_merge_request_diff
     MergeRequests::MergeRequestDiffCacheService.new.execute(self)
-
     new_diff_refs = self.diff_refs
 
     update_diff_notes_positions(

app/models/merge_request_diff.rb

@@ -8,8 +8,6 @@ class MergeRequestDiff < ActiveRecord::Base
 
   belongs_to :merge_request
 
-  delegate :source_branch_sha, :target_branch_sha, :target_branch, :source_branch, to: :merge_request, prefix: nil
-
   state_machine :state, initial: :empty do
     state :collected
     state :overflow
@@ -24,12 +22,47 @@ class MergeRequestDiff < ActiveRecord::Base
   serialize :st_commits
   serialize :st_diffs
 
-  after_create :reload_content, unless: :importing?
-  after_save :keep_around_commits, 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?
+
+  def self.select_without_diff
+    select(column_names - ['st_diffs'])
+  end
 
-  def reload_content
+  # 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
-    reload_diffs
+    save_diffs
+    keep_around_commits
+  end
+
+  def ensure_commits_sha
+    merge_request.fetch_ref
+    self.start_commit_sha ||= merge_request.target_branch_sha
+    self.head_commit_sha  ||= merge_request.source_branch_sha
+    self.base_commit_sha  ||= find_base_sha
+    save
+  end
+
+  # Override head_commit_sha to keep compatibility with merge request diff
+  # created before version 8.4 that does not store head_commit_sha in separate db field.
+  def head_commit_sha
+    if persisted? && super.nil?
+      last_commit.try(:sha)
+    else
+      super
+    end
+  end
+
+  # This method will rely on repository branch sha
+  # in case start_commit_sha is nil. Its necesarry for old merge request diff
+  # created before version 8.4 to work
+  def safe_start_commit_sha
+    start_commit_sha || merge_request.target_branch_sha
   end
 
   def size
@@ -38,14 +71,11 @@ class MergeRequestDiff < ActiveRecord::Base
 
   def raw_diffs(options = {})
     if options[:ignore_whitespace_change]
-      @raw_diffs_no_whitespace ||= begin
-        compare = Gitlab::Git::Compare.new(
+      @diffs_no_whitespace ||=
+        Gitlab::Git::Compare.new(
           repository.raw_repository,
-          self.start_commit_sha || self.target_branch_sha,
-          self.head_commit_sha || self.source_branch_sha,
-        )
-        compare.diffs(options)
-      end
+          safe_start_commit_sha,
+          head_commit_sha).diffs(options)
     else
       @raw_diffs ||= {}
       @raw_diffs[options] ||= load_diffs(st_diffs, options)
@@ -56,6 +86,11 @@ class MergeRequestDiff < ActiveRecord::Base
     @commits ||= load_commits(st_commits || [])
   end
 
+  def reload_commits
+    @commits = nil
+    commits
+  end
+
   def last_commit
     commits.first
   end
@@ -65,55 +100,60 @@ class MergeRequestDiff < ActiveRecord::Base
   end
 
   def base_commit
-    return unless self.base_commit_sha
+    return unless base_commit_sha
 
-    project.commit(self.base_commit_sha)
+    project.commit(base_commit_sha)
   end
 
   def start_commit
-    return unless self.start_commit_sha
+    return unless start_commit_sha
 
-    project.commit(self.start_commit_sha)
+    project.commit(start_commit_sha)
   end
 
   def head_commit
-    return last_commit unless self.head_commit_sha
+    return unless head_commit_sha
+
+    project.commit(head_commit_sha)
+  end
+
+  def diff_refs
+    return unless start_commit_sha || base_commit_sha
 
-    project.commit(self.head_commit_sha)
+    Gitlab::Diff::DiffRefs.new(
+      base_sha:  base_commit_sha,
+      start_sha: start_commit_sha,
+      head_sha:  head_commit_sha
+    )
   end
 
   def diff_refs_by_sha?
     base_commit_sha? && head_commit_sha? && start_commit_sha?
   end
 
+  def diffs(diff_options = nil)
+    Gitlab::Diff::FileCollection::MergeRequestDiff.new(self, diff_options: diff_options)
+  end
+
+  def project
+    merge_request.target_project
+  end
+
   def compare
     @compare ||=
-      begin
-        # Update ref for merge request
-        merge_request.fetch_ref
+      Gitlab::Git::Compare.new(
+        repository.raw_repository,
+        safe_start_commit_sha,
+        head_commit_sha
+      )
+  end
 
-        Gitlab::Git::Compare.new(
-          repository.raw_repository,
-          self.target_branch_sha,
-          self.source_branch_sha
-        )
-      end
+  def latest?
+    self == merge_request.merge_request_diff
   end
 
   private
 
-  # Collect array of Git::Commit objects
-  # between target and source branches
-  def unmerged_commits
-    commits = compare.commits
-
-    if commits.present?
-      commits = Commit.decorate(commits, merge_request.source_project).reverse
-    end
-
-    commits
-  end
-
   def dump_commits(commits)
     commits.map(&:to_hash)
   end
@@ -122,26 +162,21 @@ class MergeRequestDiff < ActiveRecord::Base
     array.map { |hash| Commit.new(Gitlab::Git::Commit.new(hash), merge_request.source_project) }
   end
 
-  # Reload all commits related to current merge request from repo
+  # Load all commits related to current merge request diff from repo
   # and save it as array of hashes in st_commits db field
-  def reload_commits
+  def save_commits
     new_attributes = {}
 
-    commit_objects = unmerged_commits
+    commits = compare.commits
 
-    if commit_objects.present?
-      new_attributes[:st_commits] = dump_commits(commit_objects)
+    if commits.present?
+      commits = Commit.decorate(commits, merge_request.source_project).reverse
+      new_attributes[:st_commits] = dump_commits(commits)
     end
 
     update_columns_serialized(new_attributes)
   end
 
-  # Collect array of Git::Diff objects
-  # between target and source branches
-  def unmerged_diffs
-    compare.diffs(Commit.max_diff_options)
-  end
-
   def dump_diffs(diffs)
     if diffs.respond_to?(:map)
       diffs.map(&:to_hash)
@@ -162,16 +197,16 @@ class MergeRequestDiff < ActiveRecord::Base
     end
   end
 
-  # Reload diffs between branches related to current merge request from repo
+  # Load diffs between branches related to current merge request diff from repo
   # and save it as array of hashes in st_diffs db field
-  def reload_diffs
+  def save_diffs
     new_attributes = {}
     new_diffs = []
 
     if commits.size.zero?
       new_attributes[:state] = :empty
     else
-      diff_collection = unmerged_diffs
+      diff_collection = compare.diffs(Commit.max_diff_options)
 
       if diff_collection.overflow?
         # Set our state to 'overflow' to make the #empty? and #collected?
@@ -188,32 +223,17 @@ class MergeRequestDiff < ActiveRecord::Base
     end
 
     new_attributes[:st_diffs] = new_diffs
-
-    new_attributes[:start_commit_sha] = self.target_branch_sha
-    new_attributes[:head_commit_sha] = self.source_branch_sha
-    new_attributes[:base_commit_sha] = branch_base_sha
-
     update_columns_serialized(new_attributes)
-
-    keep_around_commits
-  end
-
-  def project
-    merge_request.target_project
   end
 
   def repository
     project.repository
   end
 
-  def branch_base_commit
-    return unless self.source_branch_sha && self.target_branch_sha
-
-    project.merge_base_commit(self.source_branch_sha, self.target_branch_sha)
-  end
+  def find_base_sha
+    return unless head_commit_sha && start_commit_sha
 
-  def branch_base_sha
-    branch_base_commit.try(:sha)
+    project.merge_base_commit(head_commit_sha, start_commit_sha).try(:sha)
   end
 
   def utf8_st_diffs
@@ -248,8 +268,8 @@ class MergeRequestDiff < ActiveRecord::Base
   end
 
   def keep_around_commits
-    repository.keep_around(target_branch_sha)
-    repository.keep_around(source_branch_sha)
-    repository.keep_around(branch_base_sha)
+    repository.keep_around(start_commit_sha)
+    repository.keep_around(head_commit_sha)
+    repository.keep_around(base_commit_sha)
   end
 end

app/views/projects/merge_requests/show/_diffs.html.haml

 - if @merge_request_diff.collected?
+  = render 'projects/merge_requests/show/versions'
   = render "projects/diffs/diffs", diffs: @diffs
 - elsif @merge_request_diff.empty?
   .nothing-here-block Nothing to merge from #{@merge_request.source_branch} into #{@merge_request.target_branch}

app/views/projects/merge_requests/show/_versions.html.haml

+- merge_request_diffs = @merge_request.merge_request_diffs.select_without_diff
+
+- if merge_request_diffs.size > 1
+  .mr-version-switch
+    Version:
+    %span.dropdown.inline
+      %a.btn-link.dropdown-toggle{ data: {toggle: :dropdown} }
+        %strong.monospace<
+          - if @merge_request_diff.latest?
+            #{"latest"}
+          - else
+            #{@merge_request_diff.head_commit.short_id}
+        %span.caret
+      %ul.dropdown-menu.dropdown-menu-selectable
+        - merge_request_diffs.each do |merge_request_diff|
+          %li
+            = link_to diffs_namespace_project_merge_request_path(@project.namespace, @project, @merge_request, diff_id: merge_request_diff.id), class: ('is-active' if merge_request_diff == @merge_request_diff) do
+              %strong.monospace
+                #{merge_request_diff.head_commit.short_id}
+              %br
+              %small
+                #{number_with_delimiter(merge_request_diff.commits.count)} #{'commit'.pluralize(merge_request_diff.commits.count)},
+                = time_ago_with_tooltip(merge_request_diff.created_at)
+
+    - unless @merge_request_diff.latest?
+      %span.prepend-left-default
+        = icon('info-circle')
+        This version is not the latest one. Comments are disabled
+    .pull-right
+      %span.monospace
+        #{@merge_request_diff.base_commit.short_id}..#{@merge_request_diff.head_commit.short_id}

db/migrate/20160725104020_merge_request_diff_remove_uniq.rb

+# See http://doc.gitlab.com/ce/development/migration_style_guide.html
+# for more information on how to write migrations for GitLab.
+
+class MergeRequestDiffRemoveUniq < ActiveRecord::Migration
+  include Gitlab::Database::MigrationHelpers
+  disable_ddl_transaction!
+
+  DOWNTIME = false
+
+  def up
+    if index_exists?(:merge_request_diffs, :merge_request_id)
+      remove_index :merge_request_diffs, :merge_request_id
+    end
+  end
+
+  def down
+    unless index_exists?(:merge_request_diffs, :merge_request_id)
+      add_concurrent_index :merge_request_diffs, :merge_request_id, unique: true
+    end
+  end
+end

db/migrate/20160725104452_merge_request_diff_add_index.rb

+class MergeRequestDiffAddIndex < ActiveRecord::Migration
+  include Gitlab::Database::MigrationHelpers
+  disable_ddl_transaction!
+
+  # Set this constant to true if this migration requires downtime.
+  DOWNTIME = false
+
+  def up
+    add_concurrent_index :merge_request_diffs, :merge_request_id
+  end
+
+  def down
+    if index_exists?(:merge_request_diffs, :merge_request_id)
+      remove_index :merge_request_diffs, :merge_request_id
+    end
+  end
+end

db/schema.rb

@@ -593,7 +593,7 @@ ActiveRecord::Schema.define(version: 20160823081327) do
     t.string   "start_commit_sha"
   end
 
-  add_index "merge_request_diffs", ["merge_request_id"], name: "index_merge_request_diffs_on_merge_request_id", unique: true, using: :btree
+  add_index "merge_request_diffs", ["merge_request_id"], name: "index_merge_request_diffs_on_merge_request_id", using: :btree
 
   create_table "merge_requests", force: :cascade do |t|
     t.string   "target_branch",                                null: false
@@ -620,7 +620,6 @@ ActiveRecord::Schema.define(version: 20160823081327) do
     t.string   "merge_commit_sha"
     t.datetime "deleted_at"
     t.string   "in_progress_merge_commit_sha"
-    t.integer  "lock_version"
   end
 
   add_index "merge_requests", ["assignee_id"], name: "index_merge_requests_on_assignee_id", using: :btree

doc/api/merge_requests.md

@@ -902,3 +902,112 @@ Example response:
   "created_at": "2016-07-01T11:14:15.530Z"
 }
 ```
+
+## Get MR diff versions
+
+Get a list of merge request diff versions.
+
+```
+GET /projects/:id/merge_requests/:merge_request_id/versions
+```
+
+| Attribute | Type    | Required | Description           |
+| --------- | ------- | -------- | --------------------- |
+| `id`      | String  | yes      | The ID of the project |
+| `merge_request_id` | integer | yes | The ID of the merge request |
+
+```bash
+curl --header "PRIVATE-TOKEN: 9koXpg98eAheJpvBs5tK" https://gitlab.example.com/api/v3/projects/1/merge_requests/1/versions
+```
+
+Example response:
+
+```json
+[{
+  "id": 110,
+  "head_commit_sha": "33e2ee8579fda5bc36accc9c6fbd0b4fefda9e30",
+  "base_commit_sha": "eeb57dffe83deb686a60a71c16c32f71046868fd",
+  "start_commit_sha": "eeb57dffe83deb686a60a71c16c32f71046868fd",
+  "created_at": "2016-07-26T14:44:48.926Z",
+  "merge_request_id": 105,
+  "state": "collected",
+  "real_size": "1"
+}, {
+  "id": 108,
+  "head_commit_sha": "3eed087b29835c48015768f839d76e5ea8f07a24",
+  "base_commit_sha": "eeb57dffe83deb686a60a71c16c32f71046868fd",
+  "start_commit_sha": "eeb57dffe83deb686a60a71c16c32f71046868fd",
+  "created_at": "2016-07-25T14:21:33.028Z",
+  "merge_request_id": 105,
+  "state": "collected",
+  "real_size": "1"
+}]
+```
+
+## Get a single MR diff version
+
+Get a single merge request diff version.
+
+```
+GET /projects/:id/merge_requests/:merge_request_id/versions/:version_id
+```
+
+| Attribute | Type    | Required | Description           |
+| --------- | ------- | -------- | --------------------- |
+| `id`      | String  | yes      | The ID of the project |
+| `merge_request_id` | integer | yes | The ID of the merge request |
+| `version_id` | integer | yes | The ID of the merge request diff version |
+
+```bash
+curl --header "PRIVATE-TOKEN: 9koXpg98eAheJpvBs5tK" https://gitlab.example.com/api/v3/projects/1/merge_requests/1/versions/1
+```
+
+Example response:
+
+```json
+{
+  "id": 110,
+  "head_commit_sha": "33e2ee8579fda5bc36accc9c6fbd0b4fefda9e30",
+  "base_commit_sha": "eeb57dffe83deb686a60a71c16c32f71046868fd",
+  "start_commit_sha": "eeb57dffe83deb686a60a71c16c32f71046868fd",
+  "created_at": "2016-07-26T14:44:48.926Z",
+  "merge_request_id": 105,
+  "state": "collected",
+  "real_size": "1",
+  "commits": [{
+    "id": "33e2ee8579fda5bc36accc9c6fbd0b4fefda9e30",
+    "short_id": "33e2ee85",
+    "title": "Change year to 2018",
+    "author_name": "Administrator",
+    "author_email": "admin@example.com",
+    "created_at": "2016-07-26T17:44:29.000+03:00",
+    "message": "Change year to 2018"
+  }, {
+    "id": "aa24655de48b36335556ac8a3cd8bb521f977cbd",
+    "short_id": "aa24655d",
+    "title": "Update LICENSE",
+    "author_name": "Administrator",
+    "author_email": "admin@example.com",
+    "created_at": "2016-07-25T17:21:53.000+03:00",
+    "message": "Update LICENSE"
+  }, {
+    "id": "3eed087b29835c48015768f839d76e5ea8f07a24",
+    "short_id": "3eed087b",
+    "title": "Add license",
+    "author_name": "Administrator",
+    "author_email": "admin@example.com",
+    "created_at": "2016-07-25T17:21:20.000+03:00",
+    "message": "Add license"
+  }],
+  "diffs": [{
+    "old_path": "LICENSE",
+    "new_path": "LICENSE",
+    "a_mode": "0",
+    "b_mode": "100644",
+    "diff": "--- /dev/null\n+++ b/LICENSE\n@@ -0,0 +1,21 @@\n+The MIT License (MIT)\n+\n+Copyright (c) 2018 Administrator\n+\n+Permission is hereby granted, free of charge, to any person obtaining a copy\n+of this software and associated documentation files (the \"Software\"), to deal\n+in the Software without restriction, including without limitation the rights\n+to use, copy, modify, merge, publish, distribute, sublicense, and/or sell\n+copies of the Software, and to permit persons to whom the Software is\n+furnished to do so, subject to the following conditions:\n+\n+The above copyright notice and this permission notice shall be included in all\n+copies or substantial portions of the Software.\n+\n+THE SOFTWARE IS PROVIDED \"AS IS\", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR\n+IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,\n+FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE\n+AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER\n+LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,\n+OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE\n+SOFTWARE.\n",
+    "new_file": true,
+    "renamed_file": false,
+    "deleted_file": false
+  }]
+}
+```

doc/workflow/merge_requests.md

@@ -80,3 +80,11 @@ If you click the "Hide whitespace changes" button, you can see the diff without
 It is also working on commits compare view.
 
 ![Commit Compare](merge_requests/commit_compare.png)
+
+## Merge Requests versions
+
+Every time you push to merge request branch, a new version of merge request diff
+is created. When you visit the merge request page you see latest version of changes.
+However you can select an older one from version dropdown
+
+![Merge Request Versions](merge_requests/versions.png)

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

features/steps/project/merge_requests.rb

@@ -58,8 +58,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
@@ -124,7 +124,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"
           )

lib/api/api.rb

@@ -67,5 +67,6 @@ module API
     mount ::API::Triggers
     mount ::API::Users
     mount ::API::Variables
+    mount ::API::MergeRequestDiffs
   end
 end

lib/api/entities.rb

@@ -250,6 +250,19 @@ module API
       end
     end
 
+    class MergeRequestDiff < Grape::Entity
+      expose :id, :head_commit_sha, :base_commit_sha, :start_commit_sha,
+        :created_at, :merge_request_id, :state, :real_size
+    end
+
+    class MergeRequestDiffFull < MergeRequestDiff
+      expose :commits, using: Entities::RepoCommit
+
+      expose :diffs, using: Entities::RepoDiff do |compare, _|
+        compare.raw_diffs(all_diffs: true).to_a
+      end
+    end
+
     class SSHKey < Grape::Entity
       expose :id, :title, :key, :created_at
     end

lib/api/merge_request_diffs.rb

+module API
+  # MergeRequestDiff API
+  class MergeRequestDiffs < Grape::API
+    before { authenticate! }
+
+    resource :projects do
+      desc 'Get a list of merge request diff versions' do
+        detail 'This feature was introduced in GitLab 8.12.'
+        success Entities::MergeRequestDiff
+      end
+
+      params do
+        requires :id, type: String, desc: 'The ID of a project'
+        requires :merge_request_id, type: Integer, desc: 'The ID of a merge request'
+      end
+
+      get ":id/merge_requests/:merge_request_id/versions" do
+        merge_request = user_project.merge_requests.
+          find(params[:merge_request_id])
+
+        authorize! :read_merge_request, merge_request
+        present merge_request.merge_request_diffs, with: Entities::MergeRequestDiff
+      end
+
+      desc 'Get a single merge request diff version' do
+        detail 'This feature was introduced in GitLab 8.12.'
+        success Entities::MergeRequestDiffFull
+      end
+
+      params do
+        requires :id, type: String, desc: 'The ID of a project'
+        requires :merge_request_id, type: Integer, desc: 'The ID of a merge request'
+        requires :version_id, type: Integer, desc: 'The ID of a merge request diff version'
+      end
+
+      get ":id/merge_requests/:merge_request_id/versions/:version_id" do
+        merge_request = user_project.merge_requests.
+          find(params[:merge_request_id])
+
+        authorize! :read_merge_request, merge_request
+        present merge_request.merge_request_diffs.find(params[:version_id]), with: Entities::MergeRequestDiffFull
+      end
+    end
+  end
+end

lib/gitlab/diff/file_collection/merge_request.rb → lib/gitlab/diff/file_collection/merge_request_diff.rb

 module Gitlab
   module Diff
     module FileCollection
-      class MergeRequest < Base
-        def initialize(merge_request, diff_options:)
-          @merge_request = merge_request
+      class MergeRequestDiff < Base
+        def initialize(merge_request_diff, diff_options:)
+          @merge_request_diff = merge_request_diff
 
-          super(merge_request,
-            project: merge_request.project,
+          super(merge_request_diff,
+            project: merge_request_diff.project,
             diff_options: diff_options,
-            diff_refs: merge_request.diff_refs)
+            diff_refs: merge_request_diff.diff_refs)
         end
 
         def diff_files
@@ -61,11 +61,11 @@ module Gitlab
         end
 
         def cacheable?
-          @merge_request.merge_request_diff.present?
+          @merge_request_diff.present?
         end
 
         def cache_key
-          [@merge_request.merge_request_diff, 'highlighted-diff-files', diff_options]
+          [@merge_request_diff, 'highlighted-diff-files', diff_options]
         end
       end
     end

spec/features/merge_requests/merge_request_versions_spec.rb

+require 'spec_helper'
+
+feature 'Merge Request versions', js: true, feature: true do
+  before do
+    login_as :admin
+    merge_request = create(:merge_request, importing: true)
+    merge_request.merge_request_diffs.create(head_commit_sha: '6f6d7e7ed97bb5f0054f2b1df789b39ca89b6ff9')
+    merge_request.merge_request_diffs.create(head_commit_sha: '5937ac0a7beb003549fc5fd26fc247adbce4a52e')
+    project = merge_request.source_project
+    visit diffs_namespace_project_merge_request_path(project.namespace, project, merge_request)
+  end
+
+  it 'show the latest version of the diff' do
+    page.within '.mr-version-switch' do
+      expect(page).to have_content 'Version: latest'
+    end
+
+    expect(page).to have_content '8 changed files'
+  end
+
+  describe 'switch between versions' do
+    before do
+      page.within '.mr-version-switch' do
+        find('.btn-link').click
+        click_link '6f6d7e7e'
+      end
+    end
+
+    it 'should show older version' do
+      page.within '.mr-version-switch' do
+        expect(page).to have_content 'Version: 6f6d7e7e'
+      end
+
+      expect(page).to have_content '5 changed files'
+    end
+  end
+end

spec/models/merge_request_diff_spec.rb

 require 'spec_helper'
 
 describe MergeRequestDiff, models: true do
+  describe 'create new record' do
+    subject { create(:merge_request).merge_request_diff }
+
+    it { expect(subject).to be_valid }
+    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 '#latest' do
+    let!(:mr) { create(:merge_request, :with_diffs) }
+    let!(:first_diff) { mr.merge_request_diff }
+    let!(:last_diff) { mr.create_merge_request_diff }
+
+    it { expect(last_diff.latest?).to be_truthy }
+    it { expect(first_diff.latest?).to be_falsey }
+  end
+
   describe '#diffs' do
     let(:mr) { create(:merge_request, :with_diffs) }
     let(:mr_diff) { mr.merge_request_diff }

spec/models/merge_request_spec.rb

@@ -9,7 +9,7 @@ describe MergeRequest, models: true do
     it { is_expected.to belong_to(:target_project).with_foreign_key(:target_project_id).class_name('Project') }
     it { is_expected.to belong_to(:source_project).with_foreign_key(:source_project_id).class_name('Project') }
     it { is_expected.to belong_to(:merge_user).class_name("User") }
-    it { is_expected.to have_one(:merge_request_diff).dependent(:destroy) }
+    it { is_expected.to have_many(:merge_request_diffs).dependent(:destroy) }
   end
 
   describe 'modules' do
@@ -159,7 +159,7 @@ describe MergeRequest, models: true do
 
     context 'when there are MR diffs' do
       it 'delegates to the MR diffs' do
-        merge_request.merge_request_diff = MergeRequestDiff.new
+        merge_request.save
 
         expect(merge_request.merge_request_diff).to receive(:raw_diffs).with(hash_including(options))
 
@@ -316,7 +316,7 @@ describe MergeRequest, models: true do
     end
 
     it "can be removed if the last commit is the head of the source branch" do
-      allow(subject.source_project).to receive(:commit).and_return(subject.diff_head_commit)
+      allow(subject).to receive(:source_branch_head).and_return(subject.diff_head_commit)
 
       expect(subject.can_remove_source_branch?(user)).to be_truthy
     end
@@ -721,12 +721,15 @@ describe MergeRequest, models: true do
 
     let(:commit) { subject.project.commit(sample_commit.id) }
 
-    it "reloads the diff content" do
-      expect(subject.merge_request_diff).to receive(:reload_content)
-
+    it "does not change existing merge request diff" do
+      expect(subject.merge_request_diff).not_to receive(:save_git_content)
       subject.reload_diff
     end
 
+    it "creates new merge request diff" do
+      expect { subject.reload_diff }.to change { subject.merge_request_diffs.count }.by(1)
+    end
+
     it "executs diff cache service" do
       expect_any_instance_of(MergeRequests::MergeRequestDiffCacheService).to receive(:execute).with(subject)
 
@@ -736,13 +739,15 @@ describe MergeRequest, models: true do
     it "updates diff note positions" do
       old_diff_refs = subject.diff_refs
 
-      merge_request_diff = subject.merge_request_diff
-
       # Update merge_request_diff so that #diff_refs will return commit.diff_refs
-      allow(merge_request_diff).to receive(:reload_content) do
-        merge_request_diff.base_commit_sha = commit.parent_id
-        merge_request_diff.start_commit_sha = commit.parent_id
-        merge_request_diff.head_commit_sha = commit.sha
+      allow(subject).to receive(:create_merge_request_diff) do
+        subject.merge_request_diffs.create(
+          base_commit_sha: commit.parent_id,
+          start_commit_sha: commit.parent_id,
+          head_commit_sha: commit.sha
+        )
+
+        subject.merge_request_diff(true)
       end
 
       expect(Notes::DiffPositionUpdateService).to receive(:new).with(
@@ -752,14 +757,31 @@ describe MergeRequest, models: true do
         new_diff_refs: commit.diff_refs,
         paths: note.position.paths
       ).and_call_original
-      expect_any_instance_of(Notes::DiffPositionUpdateService).to receive(:execute).with(note)
 
+      expect_any_instance_of(Notes::DiffPositionUpdateService).to receive(:execute).with(note)
       expect_any_instance_of(DiffNote).to receive(:save).once
 
       subject.reload_diff
     end
   end
 
+  describe '#branch_merge_base_commit' do
+    context 'source and target branch exist' do
+      it { expect(subject.branch_merge_base_commit.sha).to eq('ae73cb07c9eeaf35924a10f713b364d32b2dd34f') }
+      it { expect(subject.branch_merge_base_commit).to be_a(Commit) }
+    end
+
+    context 'when the target branch does not exist' do
+      before do
+        subject.project.repository.raw_repository.delete_branch(subject.target_branch)
+      end
+
+      it 'returns nil' do
+        expect(subject.branch_merge_base_commit).to be_nil
+      end
+    end
+  end
+
   describe "#diff_sha_refs" do
     context "with diffs" do
       subject { create(:merge_request, :with_diffs) }

spec/requests/api/merge_request_diffs_spec.rb

+require "spec_helper"
+
+describe API::API, 'MergeRequestDiffs', api: true  do
+  include ApiHelpers
+
+  let!(:user)          { create(:user) }
+  let!(:merge_request) { create(:merge_request, importing: true) }
+  let!(:project)       { merge_request.target_project }
+
+  before do
+    merge_request.merge_request_diffs.create(head_commit_sha: '6f6d7e7ed97bb5f0054f2b1df789b39ca89b6ff9')
+    merge_request.merge_request_diffs.create(head_commit_sha: '5937ac0a7beb003549fc5fd26fc247adbce4a52e')
+    project.team << [user, :master]
+  end
+
+  describe 'GET /projects/:id/merge_requests/:merge_request_id/versions' do
+    context 'valid merge request' do
+      before { get api("/projects/#{project.id}/merge_requests/#{merge_request.id}/versions", user) }
+      let(:merge_request_diff) { merge_request.merge_request_diffs.first }
+
+      it { expect(response.status).to eq 200 }
+      it { expect(json_response.size).to eq(merge_request.merge_request_diffs.size) }
+      it { expect(json_response.first['id']).to eq(merge_request_diff.id) }
+      it { expect(json_response.first['head_commit_sha']).to eq(merge_request_diff.head_commit_sha) }
+    end
+
+    it 'returns a 404 when merge_request_id not found' do
+      get api("/projects/#{project.id}/merge_requests/999/versions", user)
+      expect(response).to have_http_status(404)
+    end
+  end
+
+  describe 'GET /projects/:id/merge_requests/:merge_request_id/versions/:version_id' do
+    context 'valid merge request' do
+      before { get api("/projects/#{project.id}/merge_requests/#{merge_request.id}/versions/#{merge_request_diff.id}", user) }
+      let(:merge_request_diff) { merge_request.merge_request_diffs.first }
+
+      it { expect(response.status).to eq 200 }
+      it { expect(json_response['id']).to eq(merge_request_diff.id) }
+      it { expect(json_response['head_commit_sha']).to eq(merge_request_diff.head_commit_sha) }
+      it { expect(json_response['diffs'].size).to eq(merge_request_diff.diffs.size) }
+    end
+
+    it 'returns a 404 when merge_request_id not found' do
+      get api("/projects/#{project.id}/merge_requests/#{merge_request.id}/versions/999", user)
+      expect(response).to have_http_status(404)
+    end
+  end
+end

spec/services/merge_requests/merge_request_diff_cache_service_spec.rb

@@ -6,7 +6,7 @@ describe MergeRequests::MergeRequestDiffCacheService do
   describe '#execute' do
     it 'retrieves the diff files to cache the highlighted result' do
       merge_request = create(:merge_request)
-      cache_key = [merge_request.merge_request_diff, 'highlighted-diff-files', Gitlab::Diff::FileCollection::MergeRequest.default_options]
+      cache_key = [merge_request.merge_request_diff, 'highlighted-diff-files', Gitlab::Diff::FileCollection::MergeRequestDiff.default_options]
 
       expect(Rails.cache).to receive(:read).with(cache_key).and_return({})
       expect(Rails.cache).to receive(:write).with(cache_key, anything)