From b6b26692ea44cfeab7e8fd64b7df60852850fce2 Mon Sep 17 00:00:00 2001
From: Sean McGivern <sean@gitlab.com>
Date: Tue, 28 Jun 2016 17:25:32 +0100
Subject: [PATCH] Collapse large diffs by default
When rendering a list of diff files, skip those where the diff is over
10 KB and provide an endpoint to render individually instead.
---
CHANGELOG | 2 +
app/controllers/projects/commit_controller.rb | 20 +-
.../projects/compare_controller.rb | 18 +-
.../projects/merge_requests_controller.rb | 76 +++--
app/helpers/diff_helper.rb | 19 ++
app/models/merge_request.rb | 6 +-
app/models/merge_request_diff.rb | 6 +
app/views/projects/diffs/_content.html.haml | 25 ++
app/views/projects/diffs/_file.html.haml | 31 +-
config/routes.rb | 15 +-
lib/gitlab/diff/file.rb | 4 +
spec/controllers/commit_controller_spec.rb | 246 ---------------
.../projects/commit_controller_spec.rb | 291 +++++++++++++++++-
.../projects/compare_controller_spec.rb | 69 +++++
.../merge_requests_controller_spec.rb | 236 ++++++++++----
spec/models/merge_request_diff_spec.rb | 47 +++
spec/models/merge_request_spec.rb | 25 ++
17 files changed, 762 insertions(+), 374 deletions(-)
create mode 100644 app/views/projects/diffs/_content.html.haml
delete mode 100644 spec/controllers/commit_controller_spec.rb
create mode 100644 spec/models/merge_request_diff_spec.rb
diff --git a/CHANGELOG b/CHANGELOG
index 09f2c44e02c..680078622cf 100644
--- a/CHANGELOG
+++ b/CHANGELOG
@@ -32,6 +32,8 @@ v 8.10.0 (unreleased)
- Fix user creation with stronger minimum password requirements !4054 (nathan-pmt)
- PipelinesFinder uses git cache data
- Throttle the update of `project.pushes_since_gc` to 1 minute.
+ - Allow expanding and collapsing files in diff view (!4990)
+ - Collapse large diffs by default (!4990)
- Check for conflicts with existing Project's wiki path when creating a new project.
- Show last push widget in upstream after push to fork
- Don't instantiate a git tree on Projects show default view
diff --git a/app/controllers/projects/commit_controller.rb b/app/controllers/projects/commit_controller.rb
index 37d6521026c..810653b4264 100644
--- a/app/controllers/projects/commit_controller.rb
+++ b/app/controllers/projects/commit_controller.rb
@@ -19,7 +19,7 @@ class Projects::CommitController < Projects::ApplicationController
@grouped_diff_notes = commit.notes.grouped_diff_notes
@notes = commit.notes.non_diff_notes.fresh
-
+
Banzai::NoteRenderer.render(
@grouped_diff_notes.values.flatten + @notes,
@project,
@@ -41,6 +41,24 @@ class Projects::CommitController < Projects::ApplicationController
end
end
+ def diff_for_path
+ return git_not_found! unless commit
+
+ opts = diff_options
+ opts[:ignore_whitespace_change] = true if params[:format] == 'diff'
+
+ diffs = commit.diffs(opts.merge(paths: [params[:path]]))
+ diff_refs = [commit.parent || commit, commit]
+
+ @comments_target = {
+ noteable_type: 'Commit',
+ commit_id: @commit.id
+ }
+ @grouped_diff_notes = {}
+
+ render_diff_for_path(diffs, diff_refs, @project)
+ end
+
def builds
end
diff --git a/app/controllers/projects/compare_controller.rb b/app/controllers/projects/compare_controller.rb
index d240b9fe989..8a04f63f4d4 100644
--- a/app/controllers/projects/compare_controller.rb
+++ b/app/controllers/projects/compare_controller.rb
@@ -6,7 +6,7 @@ class Projects::CompareController < Projects::ApplicationController
# Authorize
before_action :require_non_empty_project
before_action :authorize_download_code!
- before_action :assign_ref_vars, only: [:index, :show]
+ before_action :assign_ref_vars, only: [:index, :show, :diff_for_path]
before_action :merge_request, only: [:index, :show]
def index
@@ -35,6 +35,22 @@ class Projects::CompareController < Projects::ApplicationController
end
end
+ def diff_for_path
+ compare = CompareService.new.
+ execute(@project, @head_ref, @project, @base_ref, diff_options)
+
+ return render_404 unless compare
+
+ @commit = @project.commit(@head_ref)
+ @base_commit = @project.merge_base_commit(@base_ref, @head_ref)
+ diffs = compare.diffs(diff_options.merge(paths: [params[:path]]))
+
+ @diff_notes_disabled = true
+ @grouped_diff_notes = {}
+
+ render_diff_for_path(diffs, [@base_commit, @commit], @project)
+ end
+
def create
redirect_to namespace_project_compare_path(@project.namespace, @project,
params[:from], params[:to])
diff --git a/app/controllers/projects/merge_requests_controller.rb b/app/controllers/projects/merge_requests_controller.rb
index df1943dd9bb..3fc5a319c9b 100644
--- a/app/controllers/projects/merge_requests_controller.rb
+++ b/app/controllers/projects/merge_requests_controller.rb
@@ -13,6 +13,8 @@ class Projects::MergeRequestsController < Projects::ApplicationController
before_action :validates_merge_request, only: [:show, :diffs, :commits, :builds]
before_action :define_show_vars, only: [:show, :diffs, :commits, :builds]
before_action :define_widget_vars, only: [:merge, :cancel_merge_when_build_succeeds, :merge_check]
+ before_action :define_commit_vars, only: [:diffs]
+ before_action :define_diff_comment_vars, only: [:diffs]
before_action :ensure_ref_fetched, only: [:show, :diffs, :commits, :builds]
# Allow read any merge_request
@@ -58,7 +60,7 @@ class Projects::MergeRequestsController < Projects::ApplicationController
respond_to do |format|
format.html
-
+
format.json do
render json: @merge_request
end
@@ -80,32 +82,32 @@ class Projects::MergeRequestsController < Projects::ApplicationController
def diffs
apply_diff_view_cookie!
- @commit = @merge_request.diff_head_commit
- @base_commit = @merge_request.diff_base_commit || @merge_request.likely_diff_base_commit
-
- @comments_target = {
- noteable_type: 'MergeRequest',
- noteable_id: @merge_request.id
- }
-
- @use_legacy_diff_notes = !@merge_request.support_new_diff_notes?
- @grouped_diff_notes = @merge_request.notes.grouped_diff_notes
-
- Banzai::NoteRenderer.render(
- @grouped_diff_notes.values.flatten,
- @project,
- current_user,
- @path,
- @project_wiki,
- @ref
- )
-
respond_to do |format|
format.html
format.json { render json: { html: view_to_html_string("projects/merge_requests/show/_diffs") } }
end
end
+ # With an ID param, loads the MR at that ID. Otherwise, accepts the same params as #new
+ # and uses that (unsaved) MR.
+ #
+ def diff_for_path
+ if params[:id]
+ merge_request
+ define_diff_comment_vars
+ else
+ build_merge_request
+ @diff_notes_disabled = true
+ @grouped_diff_notes = {}
+ end
+
+ define_commit_vars
+ diffs = @merge_request.diffs(diff_options.merge(paths: [params[:path]]))
+ diff_refs = @merge_request.diff_refs
+
+ render_diff_for_path(diffs, diff_refs, @merge_request.project)
+ end
+
def commits
respond_to do |format|
format.html { render 'show' }
@@ -121,8 +123,7 @@ class Projects::MergeRequestsController < Projects::ApplicationController
end
def new
- params[:merge_request] ||= ActionController::Parameters.new(source_project: @project)
- @merge_request = MergeRequests::BuildService.new(project, current_user, merge_request_params).execute
+ build_merge_request
@noteable = @merge_request
@target_branches = if @merge_request.target_project
@@ -380,6 +381,30 @@ class Projects::MergeRequestsController < Projects::ApplicationController
closes_issues
end
+ def define_commit_vars
+ @commit = @merge_request.diff_head_commit
+ @base_commit = @merge_request.diff_base_commit || @merge_request.likely_diff_base_commit
+ end
+
+ def define_diff_comment_vars
+ @comments_target = {
+ noteable_type: 'MergeRequest',
+ noteable_id: @merge_request.id
+ }
+
+ @use_legacy_diff_notes = !@merge_request.support_new_diff_notes?
+ @grouped_diff_notes = @merge_request.notes.grouped_diff_notes
+
+ Banzai::NoteRenderer.render(
+ @grouped_diff_notes.values.flatten,
+ @project,
+ current_user,
+ @path,
+ @project_wiki,
+ @ref
+ )
+ end
+
def invalid_mr
# Render special view for MR with removed source or target branch
render 'invalid'
@@ -408,4 +433,9 @@ class Projects::MergeRequestsController < Projects::ApplicationController
params[:merge_when_build_succeeds].present? &&
@merge_request.pipeline && @merge_request.pipeline.active?
end
+
+ def build_merge_request
+ params[:merge_request] ||= ActionController::Parameters.new(source_project: @project)
+ @merge_request = MergeRequests::BuildService.new(project, current_user, merge_request_params).execute
+ end
end
diff --git a/app/helpers/diff_helper.rb b/app/helpers/diff_helper.rb
index eb57516247d..d4655d60799 100644
--- a/app/helpers/diff_helper.rb
+++ b/app/helpers/diff_helper.rb
@@ -8,6 +8,25 @@ module DiffHelper
[marked_old_line, marked_new_line]
end
+ def render_diff_for_path(diffs, diff_refs, project)
+ diff_file = safe_diff_files(diffs, diff_refs).first
+
+ return render_404 unless diff_file
+
+ diff_commit = commit_for_diff(diff_file)
+ blob = project.repository.blob_for_diff(diff_commit, diff_file)
+
+ locals = {
+ diff_file: diff_file,
+ diff_commit: diff_commit,
+ diff_refs: diff_refs,
+ blob: blob,
+ project: project
+ }
+
+ render json: { html: view_to_html_string('projects/diffs/_content', locals) }
+ end
+
def diff_view
diff_views = %w(inline parallel)
diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb
index 083e93f1ee7..d5c23716b04 100644
--- a/app/models/merge_request.rb
+++ b/app/models/merge_request.rb
@@ -19,7 +19,7 @@ class MergeRequest < ActiveRecord::Base
after_create :create_merge_request_diff, unless: :importing?
after_update :update_merge_request_diff
- delegate :commits, :diffs, :real_size, to: :merge_request_diff, prefix: nil
+ delegate :commits, :real_size, to: :merge_request_diff, prefix: nil
# When this attribute is true some MR validation is ignored
# It allows us to close or modify broken merge requests
@@ -164,6 +164,10 @@ class MergeRequest < ActiveRecord::Base
merge_request_diff ? merge_request_diff.first_commit : compare_commits.first
end
+ def diffs(*args)
+ merge_request_diff ? merge_request_diff.diffs(*args) : compare.diffs(*args)
+ end
+
def diff_size
merge_request_diff.size
end
diff --git a/app/models/merge_request_diff.rb b/app/models/merge_request_diff.rb
index ba235750aeb..06b28fc5a75 100644
--- a/app/models/merge_request_diff.rb
+++ b/app/models/merge_request_diff.rb
@@ -144,6 +144,12 @@ class MergeRequestDiff < ActiveRecord::Base
def load_diffs(raw, options)
if raw.respond_to?(:each)
+ if options[:paths]
+ raw = raw.select do |diff|
+ options[:paths].include?(diff[:new_path])
+ end
+ end
+
Gitlab::Git::DiffCollection.new(raw, options)
else
Gitlab::Git::DiffCollection.new([])
diff --git a/app/views/projects/diffs/_content.html.haml b/app/views/projects/diffs/_content.html.haml
new file mode 100644
index 00000000000..832aa0c5a14
--- /dev/null
+++ b/app/views/projects/diffs/_content.html.haml
@@ -0,0 +1,25 @@
+.diff-content.diff-wrap-lines
+ - # Skip all non non-supported blobs
+ - return unless blob.respond_to?(:text?)
+ - if diff_file.too_large?
+ .nothing-here-block This diff could not be displayed because it is too large.
+ - elsif blob.only_display_raw?
+ .nothing-here-block This file is too large to display.
+ - elsif blob_text_viewable?(blob)
+ - if !project.repository.diffable?(blob)
+ .nothing-here-block This diff was suppressed by a .gitattributes entry.
+ - elsif diff_file.diff_lines.length > 0
+ - if diff_view == 'parallel'
+ = render "projects/diffs/parallel_view", diff_file: diff_file, project: project, blob: blob
+ - else
+ = render "projects/diffs/text_file", diff_file: diff_file
+ - else
+ - if diff_file.mode_changed?
+ .nothing-here-block File mode changed
+ - elsif diff_file.renamed_file
+ .nothing-here-block File moved
+ - elsif blob.image?
+ - old_blob = diff_file.old_blob(diff_commit)
+ = render "projects/diffs/image", diff_file: diff_file, old_file: old_blob, file: blob
+ - else
+ .nothing-here-block No preview for this file type
diff --git a/app/views/projects/diffs/_file.html.haml b/app/views/projects/diffs/_file.html.haml
index 3b758a1ec4e..c83ed55efe1 100644
--- a/app/views/projects/diffs/_file.html.haml
+++ b/app/views/projects/diffs/_file.html.haml
@@ -16,28 +16,9 @@
= view_file_btn(diff_commit.id, diff_file, project)
- .diff-content.diff-wrap-lines
- - # Skip all non non-supported blobs
- - return unless blob.respond_to?(:text?)
- - if diff_file.too_large?
- .nothing-here-block This diff could not be displayed because it is too large.
- - elsif blob.only_display_raw?
- .nothing-here-block This file is too large to display.
- - elsif blob_text_viewable?(blob)
- - if !project.repository.diffable?(blob)
- .nothing-here-block This diff was suppressed by a .gitattributes entry.
- - elsif diff_file.diff_lines.length > 0
- - if diff_view == 'parallel'
- = render "projects/diffs/parallel_view", diff_file: diff_file, project: project, blob: blob, index: i
- - else
- = render "projects/diffs/text_file", diff_file: diff_file, index: i
- - else
- - if diff_file.mode_changed?
- .nothing-here-block File mode changed
- - elsif diff_file.renamed_file
- .nothing-here-block File moved
- - elsif blob.image?
- - old_blob = diff_file.old_blob(diff_commit)
- = render "projects/diffs/image", diff_file: diff_file, old_file: old_blob, file: blob, index: i
- - else
- .nothing-here-block No preview for this file type
+ - if diff_file.collapsed_by_default?
+ - url = url_for(params.merge(action: :diff_for_path, path: diff_file.file_path, format: nil))
+ .diff-content.diff-wrap-lines{data: { diff_for_path: url }}
+ .nothing-here-block File hidden by default; content for this element available at #{url}
+ - else
+ = render 'projects/diffs/content', diff_file: diff_file, diff_commit: diff_commit, diff_refs: diff_refs, blob: blob, project: project
diff --git a/config/routes.rb b/config/routes.rb
index 18a4ead2b37..f31f8171993 100644
--- a/config/routes.rb
+++ b/config/routes.rb
@@ -615,10 +615,18 @@ Rails.application.routes.draw do
post :retry_builds
post :revert
post :cherry_pick
+ get '/diffs/*path', action: :diff_for_path, constraints: { format: false }
end
end
- resources :compare, only: [:index, :create]
+ resources :compare, only: [:index, :create] do
+ collection do
+ get '/diffs/*path', action: :diff_for_path, constraints: { format: false }
+ end
+ end
+
+ get '/compare/:from...:to', to: 'compare#show', as: 'compare', constraints: { from: /.+/, to: /.+/ }
+
resources :network, only: [:show], constraints: { id: /(?:[^.]|\.(?!json$))+/, format: /json/ }
resources :graphs, only: [:show], constraints: { id: /(?:[^.]|\.(?!json$))+/, format: /json/ } do
@@ -629,9 +637,6 @@ Rails.application.routes.draw do
end
end
- get '/compare/:from...:to' => 'compare#show', :as => 'compare',
- :constraints => { from: /.+/, to: /.+/ }
-
resources :snippets, constraints: { id: /\d+/ } do
member do
get 'raw'
@@ -706,12 +711,14 @@ Rails.application.routes.draw do
post :toggle_subscription
post :toggle_award_emoji
post :remove_wip
+ get '/diffs/*path', action: :diff_for_path, constraints: { format: false }
end
collection do
get :branch_from
get :branch_to
get :update_branches
+ get '/diffs/*path', action: :diff_for_path, constraints: { format: false }
end
end
diff --git a/lib/gitlab/diff/file.rb b/lib/gitlab/diff/file.rb
index b0c50edba59..7e01f7b61fb 100644
--- a/lib/gitlab/diff/file.rb
+++ b/lib/gitlab/diff/file.rb
@@ -68,6 +68,10 @@ module Gitlab
@lines ||= Gitlab::Diff::Parser.new.parse(raw_diff.each_line).to_a
end
+ def collapsed_by_default?
+ diff.diff.bytesize > 10240 # 10 KB
+ end
+
def highlighted_diff_lines
@highlighted_diff_lines ||= Gitlab::Diff::Highlight.new(self, repository: self.repository).highlight
end
diff --git a/spec/controllers/commit_controller_spec.rb b/spec/controllers/commit_controller_spec.rb
deleted file mode 100644
index a3a3309e15e..00000000000
--- a/spec/controllers/commit_controller_spec.rb
+++ /dev/null
@@ -1,246 +0,0 @@
-require 'spec_helper'
-
-describe Projects::CommitController do
- let(:project) { create(:project) }
- let(:user) { create(:user) }
- let(:commit) { project.commit("master") }
- let(:master_pickable_sha) { '7d3b0f7cff5f37573aea97cebfd5692ea1689924' }
- let(:master_pickable_commit) { project.commit(master_pickable_sha) }
-
- before do
- sign_in(user)
- project.team << [user, :master]
- end
-
- describe "#show" do
- shared_examples "export as" do |format|
- it "should generally work" do
- get(:show,
- namespace_id: project.namespace.to_param,
- project_id: project.to_param,
- id: commit.id,
- format: format)
-
- expect(response).to be_success
- end
-
- it "should generate it" do
- expect_any_instance_of(Commit).to receive(:"to_#{format}")
-
- get(:show,
- namespace_id: project.namespace.to_param,
- project_id: project.to_param,
- id: commit.id, format: format)
- end
-
- it "should render it" do
- get(:show,
- namespace_id: project.namespace.to_param,
- project_id: project.to_param,
- id: commit.id, format: format)
-
- expect(response.body).to eq(commit.send(:"to_#{format}"))
- end
-
- it "should not escape Html" do
- allow_any_instance_of(Commit).to receive(:"to_#{format}").
- and_return('HTML entities &<>" ')
-
- get(:show,
- namespace_id: project.namespace.to_param,
- project_id: project.to_param,
- id: commit.id, format: format)
-
- expect(response.body).not_to include('&')
- expect(response.body).not_to include('>')
- expect(response.body).not_to include('<')
- expect(response.body).not_to include('"')
- end
- end
-
- describe "as diff" do
- include_examples "export as", :diff
- let(:format) { :diff }
-
- it "should really only be a git diff" do
- get(:show,
- namespace_id: project.namespace.to_param,
- project_id: project.to_param,
- id: commit.id,
- format: format)
-
- expect(response.body).to start_with("diff --git")
- end
-
- it "should really only be a git diff without whitespace changes" do
- get(:show,
- namespace_id: project.namespace.to_param,
- project_id: project.to_param,
- id: '66eceea0db202bb39c4e445e8ca28689645366c5',
- # id: commit.id,
- format: format,
- w: 1)
-
- expect(response.body).to start_with("diff --git")
- # without whitespace option, there are more than 2 diff_splits
- diff_splits = assigns(:diffs).first.diff.split("\n")
- expect(diff_splits.length).to be <= 2
- end
- end
-
- describe "as patch" do
- include_examples "export as", :patch
- let(:format) { :patch }
-
- it "should really be a git email patch" do
- get(:show,
- namespace_id: project.namespace.to_param,
- project_id: project.to_param,
- id: commit.id,
- format: format)
-
- expect(response.body).to start_with("From #{commit.id}")
- end
-
- it "should contain a git diff" do
- get(:show,
- namespace_id: project.namespace.to_param,
- project_id: project.to_param,
- id: commit.id,
- format: format)
-
- expect(response.body).to match(/^diff --git/)
- end
- end
-
- context 'commit that removes a submodule' do
- render_views
-
- let(:fork_project) { create(:forked_project_with_submodules) }
- let(:commit) { fork_project.commit('remove-submodule') }
-
- before do
- fork_project.team << [user, :master]
- end
-
- it 'renders it' do
- get(:show,
- namespace_id: fork_project.namespace.to_param,
- project_id: fork_project.to_param,
- id: commit.id)
-
- expect(response).to be_success
- end
- end
- end
-
- describe "#branches" do
- it "contains branch and tags information" do
- get(:branches,
- namespace_id: project.namespace.to_param,
- project_id: project.to_param,
- id: commit.id)
-
- expect(assigns(:branches)).to include("master", "feature_conflict")
- expect(assigns(:tags)).to include("v1.1.0")
- end
- end
-
- describe '#revert' do
- context 'when target branch is not provided' do
- it 'should render the 404 page' do
- post(:revert,
- namespace_id: project.namespace.to_param,
- project_id: project.to_param,
- id: commit.id)
-
- expect(response).not_to be_success
- expect(response).to have_http_status(404)
- end
- end
-
- context 'when the revert was successful' do
- it 'should redirect to the commits page' do
- post(:revert,
- namespace_id: project.namespace.to_param,
- project_id: project.to_param,
- target_branch: 'master',
- id: commit.id)
-
- expect(response).to redirect_to namespace_project_commits_path(project.namespace, project, 'master')
- expect(flash[:notice]).to eq('The commit has been successfully reverted.')
- end
- end
-
- context 'when the revert failed' do
- before do
- post(:revert,
- namespace_id: project.namespace.to_param,
- project_id: project.to_param,
- target_branch: 'master',
- id: commit.id)
- end
-
- it 'should redirect to the commit page' do
- # Reverting a commit that has been already reverted.
- post(:revert,
- namespace_id: project.namespace.to_param,
- project_id: project.to_param,
- target_branch: 'master',
- id: commit.id)
-
- expect(response).to redirect_to namespace_project_commit_path(project.namespace, project, commit.id)
- expect(flash[:alert]).to match('Sorry, we cannot revert this commit automatically.')
- end
- end
- end
-
- describe '#cherry_pick' do
- context 'when target branch is not provided' do
- it 'should render the 404 page' do
- post(:cherry_pick,
- namespace_id: project.namespace.to_param,
- project_id: project.to_param,
- id: master_pickable_commit.id)
-
- expect(response).not_to be_success
- expect(response).to have_http_status(404)
- end
- end
-
- context 'when the cherry-pick was successful' do
- it 'should redirect to the commits page' do
- post(:cherry_pick,
- namespace_id: project.namespace.to_param,
- project_id: project.to_param,
- target_branch: 'master',
- id: master_pickable_commit.id)
-
- expect(response).to redirect_to namespace_project_commits_path(project.namespace, project, 'master')
- expect(flash[:notice]).to eq('The commit has been successfully cherry-picked.')
- end
- end
-
- context 'when the cherry_pick failed' do
- before do
- post(:cherry_pick,
- namespace_id: project.namespace.to_param,
- project_id: project.to_param,
- target_branch: 'master',
- id: master_pickable_commit.id)
- end
-
- it 'should redirect to the commit page' do
- # Cherry-picking a commit that has been already cherry-picked.
- post(:cherry_pick,
- namespace_id: project.namespace.to_param,
- project_id: project.to_param,
- target_branch: 'master',
- id: master_pickable_commit.id)
-
- expect(response).to redirect_to namespace_project_commit_path(project.namespace, project, master_pickable_commit.id)
- expect(flash[:alert]).to match('Sorry, we cannot cherry-pick this commit automatically.')
- end
- end
- end
-end
diff --git a/spec/controllers/projects/commit_controller_spec.rb b/spec/controllers/projects/commit_controller_spec.rb
index 6e3db10e451..9679b4f849f 100644
--- a/spec/controllers/projects/commit_controller_spec.rb
+++ b/spec/controllers/projects/commit_controller_spec.rb
@@ -1,9 +1,29 @@
-require 'rails_helper'
+require 'spec_helper'
describe Projects::CommitController do
- describe 'GET show' do
+ let(:project) { create(:project) }
+ let(:user) { create(:user) }
+ let(:commit) { project.commit("master") }
+ let(:master_pickable_sha) { '7d3b0f7cff5f37573aea97cebfd5692ea1689924' }
+ let(:master_pickable_commit) { project.commit(master_pickable_sha) }
+
+ before do
+ sign_in(user)
+ project.team << [user, :master]
+ end
+
+ describe 'GET #show' do
render_views
+ def go(extra_params = {})
+ params = {
+ namespace_id: project.namespace.to_param,
+ project_id: project.to_param
+ }
+
+ get :show, params.merge(extra_params)
+ end
+
let(:project) { create(:project) }
before do
@@ -15,7 +35,7 @@ describe Projects::CommitController do
context 'with valid id' do
it 'responds with 200' do
- go id: project.commit.id
+ go(id: commit.id)
expect(response).to be_ok
end
@@ -23,27 +43,274 @@ describe Projects::CommitController do
context 'with invalid id' do
it 'responds with 404' do
- go id: project.commit.id.reverse
+ go(id: commit.id.reverse)
expect(response).to be_not_found
end
end
it 'handles binary files' do
- get(:show,
+ go(id: TestEnv::BRANCH_SHA['binary-encoding'], format: 'html')
+
+ expect(response).to be_success
+ end
+
+ shared_examples "export as" do |format|
+ it "should generally work" do
+ go(id: commit.id, format: format)
+
+ expect(response).to be_success
+ end
+
+ it "should generate it" do
+ expect_any_instance_of(Commit).to receive(:"to_#{format}")
+
+ go(id: commit.id, format: format)
+ end
+
+ it "should render it" do
+ go(id: commit.id, format: format)
+
+ expect(response.body).to eq(commit.send(:"to_#{format}"))
+ end
+
+ it "should not escape Html" do
+ allow_any_instance_of(Commit).to receive(:"to_#{format}").
+ and_return('HTML entities &<>" ')
+
+ go(id: commit.id, format: format)
+
+ expect(response.body).not_to include('&')
+ expect(response.body).not_to include('>')
+ expect(response.body).not_to include('<')
+ expect(response.body).not_to include('"')
+ end
+ end
+
+ describe "as diff" do
+ include_examples "export as", :diff
+ let(:format) { :diff }
+
+ it "should really only be a git diff" do
+ go(id: commit.id, format: format)
+
+ expect(response.body).to start_with("diff --git")
+ end
+
+ it "should really only be a git diff without whitespace changes" do
+ go(id: '66eceea0db202bb39c4e445e8ca28689645366c5', format: format, w: 1)
+
+ expect(response.body).to start_with("diff --git")
+ # without whitespace option, there are more than 2 diff_splits
+ diff_splits = assigns(:diffs).first.diff.split("\n")
+ expect(diff_splits.length).to be <= 2
+ end
+ end
+
+ describe "as patch" do
+ include_examples "export as", :patch
+ let(:format) { :patch }
+
+ it "should really be a git email patch" do
+ go(id: commit.id, format: format)
+
+ expect(response.body).to start_with("From #{commit.id}")
+ end
+
+ it "should contain a git diff" do
+ go(id: commit.id, format: format)
+
+ expect(response.body).to match(/^diff --git/)
+ end
+ end
+
+ context 'commit that removes a submodule' do
+ render_views
+
+ let(:fork_project) { create(:forked_project_with_submodules, visibility_level: 20) }
+ let(:commit) { fork_project.commit('remove-submodule') }
+
+ it 'renders it' do
+ get(:show,
+ namespace_id: fork_project.namespace.to_param,
+ project_id: fork_project.to_param,
+ id: commit.id)
+
+ expect(response).to be_success
+ end
+ end
+ end
+
+ describe "#branches" do
+ it "contains branch and tags information" do
+ get(:branches,
namespace_id: project.namespace.to_param,
project_id: project.to_param,
- id: TestEnv::BRANCH_SHA['binary-encoding'],
- format: "html")
+ id: commit.id)
- expect(response).to be_success
+ expect(assigns(:branches)).to include("master", "feature_conflict")
+ expect(assigns(:tags)).to include("v1.1.0")
+ end
+ end
+
+ describe '#revert' do
+ context 'when target branch is not provided' do
+ it 'should render the 404 page' do
+ post(:revert,
+ namespace_id: project.namespace.to_param,
+ project_id: project.to_param,
+ id: commit.id)
+
+ expect(response).not_to be_success
+ expect(response).to have_http_status(404)
+ end
+ end
+
+ context 'when the revert was successful' do
+ it 'should redirect to the commits page' do
+ post(:revert,
+ namespace_id: project.namespace.to_param,
+ project_id: project.to_param,
+ target_branch: 'master',
+ id: commit.id)
+
+ expect(response).to redirect_to namespace_project_commits_path(project.namespace, project, 'master')
+ expect(flash[:notice]).to eq('The commit has been successfully reverted.')
+ end
+ end
+
+ context 'when the revert failed' do
+ before do
+ post(:revert,
+ namespace_id: project.namespace.to_param,
+ project_id: project.to_param,
+ target_branch: 'master',
+ id: commit.id)
+ end
+
+ it 'should redirect to the commit page' do
+ # Reverting a commit that has been already reverted.
+ post(:revert,
+ namespace_id: project.namespace.to_param,
+ project_id: project.to_param,
+ target_branch: 'master',
+ id: commit.id)
+
+ expect(response).to redirect_to namespace_project_commit_path(project.namespace, project, commit.id)
+ expect(flash[:alert]).to match('Sorry, we cannot revert this commit automatically.')
+ end
+ end
+ end
+
+ describe '#cherry_pick' do
+ context 'when target branch is not provided' do
+ it 'should render the 404 page' do
+ post(:cherry_pick,
+ namespace_id: project.namespace.to_param,
+ project_id: project.to_param,
+ id: master_pickable_commit.id)
+
+ expect(response).not_to be_success
+ expect(response).to have_http_status(404)
+ end
+ end
+
+ context 'when the cherry-pick was successful' do
+ it 'should redirect to the commits page' do
+ post(:cherry_pick,
+ namespace_id: project.namespace.to_param,
+ project_id: project.to_param,
+ target_branch: 'master',
+ id: master_pickable_commit.id)
+
+ expect(response).to redirect_to namespace_project_commits_path(project.namespace, project, 'master')
+ expect(flash[:notice]).to eq('The commit has been successfully cherry-picked.')
+ end
end
- def go(id:)
- get :show,
+ context 'when the cherry_pick failed' do
+ before do
+ post(:cherry_pick,
+ namespace_id: project.namespace.to_param,
+ project_id: project.to_param,
+ target_branch: 'master',
+ id: master_pickable_commit.id)
+ end
+
+ it 'should redirect to the commit page' do
+ # Cherry-picking a commit that has been already cherry-picked.
+ post(:cherry_pick,
+ namespace_id: project.namespace.to_param,
+ project_id: project.to_param,
+ target_branch: 'master',
+ id: master_pickable_commit.id)
+
+ expect(response).to redirect_to namespace_project_commit_path(project.namespace, project, master_pickable_commit.id)
+ expect(flash[:alert]).to match('Sorry, we cannot cherry-pick this commit automatically.')
+ end
+ end
+ end
+
+ describe 'GET #diff_for_path' do
+ def diff_for_path(extra_params = {})
+ params = {
namespace_id: project.namespace.to_param,
- project_id: project.to_param,
- id: id
+ project_id: project.to_param
+ }
+
+ get :diff_for_path, params.merge(extra_params)
+ end
+
+ let(:existing_path) { '.gitmodules' }
+
+ context 'when the commit exists' do
+ context 'when the user has access to the project' do
+ context 'when the path exists in the diff' do
+ it 'enables diff notes' do
+ diff_for_path(id: commit.id, path: existing_path)
+
+ expect(assigns(:diff_notes_disabled)).to be_falsey
+ expect(assigns(:comments_target)).to eq(noteable_type: 'Commit',
+ commit_id: commit.id)
+ end
+
+ it 'only renders the diffs for the path given' do
+ expect(controller).to receive(:render_diff_for_path).and_wrap_original do |meth, diffs, diff_refs, project|
+ expect(diffs.map(&:new_path)).to contain_exactly(existing_path)
+ meth.call(diffs, diff_refs, project)
+ end
+
+ diff_for_path(id: commit.id, path: existing_path)
+ end
+ end
+
+ context 'when the path does not exist in the diff' do
+ before { diff_for_path(id: commit.id, path: existing_path.succ) }
+
+ it 'returns a 404' do
+ expect(response).to have_http_status(404)
+ end
+ end
+ end
+
+ context 'when the user does not have access to the project' do
+ before do
+ project.team.truncate
+ diff_for_path(id: commit.id, path: existing_path)
+ end
+
+ it 'returns a 404' do
+ expect(response).to have_http_status(404)
+ end
+ end
+ end
+
+ context 'when the commit does not exist' do
+ before { diff_for_path(id: commit.id.succ, path: existing_path) }
+
+ it 'returns a 404' do
+ expect(response).to have_http_status(404)
+ end
end
end
end
diff --git a/spec/controllers/projects/compare_controller_spec.rb b/spec/controllers/projects/compare_controller_spec.rb
index 4018dac95a2..40a068a87bb 100644
--- a/spec/controllers/projects/compare_controller_spec.rb
+++ b/spec/controllers/projects/compare_controller_spec.rb
@@ -64,4 +64,73 @@ describe Projects::CompareController do
expect(assigns(:commits)).to eq(nil)
end
end
+
+ describe 'GET #diff_for_path' do
+ def diff_for_path(extra_params = {})
+ params = {
+ namespace_id: project.namespace.to_param,
+ project_id: project.to_param
+ }
+
+ get :diff_for_path, params.merge(extra_params)
+ end
+
+ let(:existing_path) { 'files/ruby/feature.rb' }
+
+ context 'when the from and to refs exist' do
+ context 'when the user has access to the project' do
+ context 'when the path exists in the diff' do
+ it 'disables diff notes' do
+ diff_for_path(from: ref_from, to: ref_to, path: existing_path)
+
+ expect(assigns(:diff_notes_disabled)).to be_truthy
+ end
+
+ it 'only renders the diffs for the path given' do
+ expect(controller).to receive(:render_diff_for_path).and_wrap_original do |meth, diffs, diff_refs, project|
+ expect(diffs.map(&:new_path)).to contain_exactly(existing_path)
+ meth.call(diffs, diff_refs, project)
+ end
+
+ diff_for_path(from: ref_from, to: ref_to, path: existing_path)
+ end
+ end
+
+ context 'when the path does not exist in the diff' do
+ before { diff_for_path(from: ref_from, to: ref_to, path: existing_path.succ) }
+
+ it 'returns a 404' do
+ expect(response).to have_http_status(404)
+ end
+ end
+ end
+
+ context 'when the user does not have access to the project' do
+ before do
+ project.team.truncate
+ diff_for_path(from: ref_from, to: ref_to, path: existing_path)
+ end
+
+ it 'returns a 404' do
+ expect(response).to have_http_status(404)
+ end
+ end
+ end
+
+ context 'when the from ref does not exist' do
+ before { diff_for_path(from: ref_from.succ, to: ref_to, path: existing_path) }
+
+ it 'returns a 404' do
+ expect(response).to have_http_status(404)
+ end
+ end
+
+ context 'when the to ref does not exist' do
+ before { diff_for_path(from: ref_from, to: ref_to.succ, path: existing_path) }
+
+ it 'returns a 404' do
+ expect(response).to have_http_status(404)
+ end
+ end
+ end
end
diff --git a/spec/controllers/projects/merge_requests_controller_spec.rb b/spec/controllers/projects/merge_requests_controller_spec.rb
index c4b57e77804..1b4c4dcd1e5 100644
--- a/spec/controllers/projects/merge_requests_controller_spec.rb
+++ b/spec/controllers/projects/merge_requests_controller_spec.rb
@@ -289,101 +289,215 @@ describe Projects::MergeRequestsController do
end
end
- describe 'GET diffs' do
- def go(format: 'html')
- get :diffs,
- namespace_id: project.namespace.to_param,
- project_id: project.to_param,
- id: merge_request.iid,
- format: format
+ describe 'GET #diffs' do
+ def go(extra_params = {})
+ params = {
+ namespace_id: project.namespace.to_param,
+ project_id: project.to_param,
+ id: merge_request.iid
+ }
+
+ get :diffs, params.merge(extra_params)
end
- context 'as html' do
- it 'renders the diff template' do
- go
+ context 'with default params' do
+ context 'as html' do
+ before { go(format: 'html') }
- expect(response).to render_template('diffs')
+ it 'renders the diff template' do
+ expect(response).to render_template('diffs')
+ end
end
- end
- context 'as json' do
- it 'renders the diffs template to a string' do
- go format: 'json'
+ context 'as json' do
+ before { go(format: 'json') }
- expect(response).to render_template('projects/merge_requests/show/_diffs')
- expect(JSON.parse(response.body)).to have_key('html')
+ it 'renders the diffs template to a string' do
+ expect(response).to render_template('projects/merge_requests/show/_diffs')
+ expect(JSON.parse(response.body)).to have_key('html')
+ end
end
- end
-
- context 'with forked projects with submodules' do
- render_views
- let(:project) { create(:project) }
- let(:fork_project) { create(:forked_project_with_submodules) }
- let(:merge_request) { create(:merge_request_with_diffs, source_project: fork_project, source_branch: 'add-submodule-version-bump', target_branch: 'master', target_project: project) }
+ context 'with forked projects with submodules' do
+ render_views
- before do
- fork_project.build_forked_project_link(forked_to_project_id: fork_project.id, forked_from_project_id: project.id)
- fork_project.save
- merge_request.reload
- end
+ let(:project) { create(:project) }
+ let(:fork_project) { create(:forked_project_with_submodules) }
+ let(:merge_request) { create(:merge_request_with_diffs, source_project: fork_project, source_branch: 'add-submodule-version-bump', target_branch: 'master', target_project: project) }
- it 'renders' do
- go format: 'json'
+ before do
+ fork_project.build_forked_project_link(forked_to_project_id: fork_project.id, forked_from_project_id: project.id)
+ fork_project.save
+ merge_request.reload
+ go(format: 'json')
+ end
- expect(response).to be_success
- expect(response.body).to have_content('Subproject commit')
+ it 'renders' do
+ expect(response).to be_success
+ expect(response.body).to have_content('Subproject commit')
+ end
end
end
- end
- describe 'GET diffs with ignore_whitespace_change' do
- def go(format: 'html')
- get :diffs,
- namespace_id: project.namespace.to_param,
- project_id: project.to_param,
- id: merge_request.iid,
- format: format,
- w: 1
- end
+ context 'with ignore_whitespace_change' do
+ context 'as html' do
+ before { go(format: 'html', w: 1) }
- context 'as html' do
- it 'renders the diff template' do
- go
+ it 'renders the diff template' do
+ expect(response).to render_template('diffs')
+ end
+ end
+
+ context 'as json' do
+ before { go(format: 'json', w: 1) }
- expect(response).to render_template('diffs')
+ it 'renders the diffs template to a string' do
+ expect(response).to render_template('projects/merge_requests/show/_diffs')
+ expect(JSON.parse(response.body)).to have_key('html')
+ end
end
end
- context 'as json' do
- it 'renders the diffs template to a string' do
- go format: 'json'
+ context 'with view' do
+ before { go(view: 'parallel') }
- expect(response).to render_template('projects/merge_requests/show/_diffs')
- expect(JSON.parse(response.body)).to have_key('html')
+ it 'saves the preferred diff view in a cookie' do
+ expect(response.cookies['diff_view']).to eq('parallel')
end
end
end
- describe 'GET diffs with view' do
- def go(extra_params = {})
+ describe 'GET #diff_for_path' do
+ def diff_for_path(extra_params = {})
params = {
namespace_id: project.namespace.to_param,
- project_id: project.to_param,
- id: merge_request.iid
+ project_id: project.to_param
}
- get :diffs, params.merge(extra_params)
+ get :diff_for_path, params.merge(extra_params)
end
- it 'saves the preferred diff view in a cookie' do
- go view: 'parallel'
+ context 'when an ID param is passed' do
+ let(:existing_path) { 'files/ruby/popen.rb' }
- expect(response.cookies['diff_view']).to eq('parallel')
+ context 'when the merge request exists' do
+ context 'when the user can view the merge request' do
+ context 'when the path exists in the diff' do
+ it 'enables diff notes' do
+ diff_for_path(id: merge_request.iid, path: existing_path)
+
+ expect(assigns(:diff_notes_disabled)).to be_falsey
+ expect(assigns(:comments_target)).to eq(noteable_type: 'MergeRequest',
+ noteable_id: merge_request.id)
+ end
+
+ it 'only renders the diffs for the path given' do
+ expect(controller).to receive(:render_diff_for_path).and_wrap_original do |meth, diffs, diff_refs, project|
+ expect(diffs.map(&:new_path)).to contain_exactly(existing_path)
+ meth.call(diffs, diff_refs, project)
+ end
+
+ diff_for_path(id: merge_request.iid, path: existing_path)
+ end
+ end
+
+ context 'when the path does not exist in the diff' do
+ before { diff_for_path(id: merge_request.iid, path: 'files/ruby/nopen.rb') }
+
+ it 'returns a 404' do
+ expect(response).to have_http_status(404)
+ end
+ end
+ end
+
+ context 'when the user cannot view the merge request' do
+ before do
+ project.team.truncate
+ diff_for_path(id: merge_request.iid, path: existing_path)
+ end
+
+ it 'returns a 404' do
+ expect(response).to have_http_status(404)
+ end
+ end
+ end
+
+ context 'when the merge request does not exist' do
+ before { diff_for_path(id: merge_request.iid.succ, path: existing_path) }
+
+ it 'returns a 404' do
+ expect(response).to have_http_status(404)
+ end
+ end
+
+ context 'when the merge request belongs to a different project' do
+ let(:other_project) { create(:empty_project) }
+
+ before do
+ other_project.team << [user, :master]
+ diff_for_path(id: merge_request.iid, path: existing_path, project_id: other_project.to_param)
+ end
+
+ it 'returns a 404' do
+ expect(response).to have_http_status(404)
+ end
+ end
+ end
+
+ context 'when source and target params are passed' do
+ let(:existing_path) { 'files/ruby/feature.rb' }
+
+ context 'when both branches are in the same project' do
+ it 'disables diff notes' do
+ diff_for_path(path: existing_path, merge_request: { source_branch: 'feature', target_branch: 'master' })
+
+ expect(assigns(:diff_notes_disabled)).to be_truthy
+ end
+
+ it 'only renders the diffs for the path given' do
+ expect(controller).to receive(:render_diff_for_path).and_wrap_original do |meth, diffs, diff_refs, project|
+ expect(diffs.map(&:new_path)).to contain_exactly(existing_path)
+ meth.call(diffs, diff_refs, project)
+ end
+
+ diff_for_path(path: existing_path, merge_request: { source_branch: 'feature', target_branch: 'master' })
+ end
+ end
+
+ context 'when the source branch is in a different project to the target' do
+ let(:other_project) { create(:project) }
+
+ before { other_project.team << [user, :master] }
+
+ context 'when the path exists in the diff' do
+ it 'disables diff notes' do
+ diff_for_path(path: existing_path, merge_request: { source_project: other_project, source_branch: 'feature', target_branch: 'master' })
+
+ expect(assigns(:diff_notes_disabled)).to be_truthy
+ end
+
+ it 'only renders the diffs for the path given' do
+ expect(controller).to receive(:render_diff_for_path).and_wrap_original do |meth, diffs, diff_refs, project|
+ expect(diffs.map(&:new_path)).to contain_exactly(existing_path)
+ meth.call(diffs, diff_refs, project)
+ end
+
+ diff_for_path(path: existing_path, merge_request: { source_project: other_project, source_branch: 'feature', target_branch: 'master' })
+ end
+ end
+
+ context 'when the path does not exist in the diff' do
+ before { diff_for_path(path: 'files/ruby/nopen.rb', merge_request: { source_project: other_project, source_branch: 'feature', target_branch: 'master' }) }
+
+ it 'returns a 404' do
+ expect(response).to have_http_status(404)
+ end
+ end
+ end
end
end
- describe 'GET commits' do
+ describe 'GET #commits' do
def go(format: 'html')
get :commits,
namespace_id: project.namespace.to_param,
diff --git a/spec/models/merge_request_diff_spec.rb b/spec/models/merge_request_diff_spec.rb
new file mode 100644
index 00000000000..3f5763b6408
--- /dev/null
+++ b/spec/models/merge_request_diff_spec.rb
@@ -0,0 +1,47 @@
+require 'spec_helper'
+
+describe MergeRequestDiff, models: true do
+ describe '#diffs' do
+ let(:mr) { create(:merge_request, :with_diffs) }
+ let(:mr_diff) { mr.merge_request_diff }
+
+ context 'when the :ignore_whitespace_change option is set' do
+ it 'creates a new compare object instead of loading from the DB' do
+ expect(mr_diff).not_to receive(:load_diffs)
+ expect(Gitlab::Git::Compare).to receive(:new).and_call_original
+
+ mr_diff.diffs(ignore_whitespace_change: true)
+ end
+ end
+
+ context 'when the raw diffs are empty' do
+ before { mr_diff.update_attributes(st_diffs: '') }
+
+ it 'returns an empty DiffCollection' do
+ expect(mr_diff.diffs).to be_a(Gitlab::Git::DiffCollection)
+ expect(mr_diff.diffs).to be_empty
+ end
+ end
+
+ context 'when the raw diffs exist' do
+ it 'returns the diffs' do
+ expect(mr_diff.diffs).to be_a(Gitlab::Git::DiffCollection)
+ expect(mr_diff.diffs).not_to be_empty
+ end
+
+ context 'when the :paths option is set' do
+ let(:diffs) { mr_diff.diffs(paths: ['.gitignore', 'files/ruby/popen.rb', 'files/ruby/string.rb']) }
+
+ it 'only returns diffs that match the paths given' do
+ expect(diffs.map(&:new_path)).to contain_exactly('.gitignore', 'files/ruby/popen.rb')
+ end
+
+ it 'uses the diffs from the DB' do
+ expect(mr_diff).to receive(:load_diffs)
+
+ diffs
+ end
+ end
+ end
+ end
+end
diff --git a/spec/models/merge_request_spec.rb b/spec/models/merge_request_spec.rb
index a4b6ff8f8ad..c8ad7ab3e7f 100644
--- a/spec/models/merge_request_spec.rb
+++ b/spec/models/merge_request_spec.rb
@@ -116,6 +116,31 @@ describe MergeRequest, models: true do
end
end
+ describe '#diffs' do
+ let(:merge_request) { build(:merge_request) }
+ let(:options) { { paths: ['a/b', 'b/a', 'c/*'] } }
+
+ context 'when there are MR diffs' do
+ it 'delegates to the MR diffs' do
+ merge_request.merge_request_diff = MergeRequestDiff.new
+
+ expect(merge_request.merge_request_diff).to receive(:diffs).with(options)
+
+ merge_request.diffs(options)
+ end
+ end
+
+ context 'when there are no MR diffs' do
+ it 'delegates to the compare object' do
+ merge_request.compare = double(:compare)
+
+ expect(merge_request.compare).to receive(:diffs).with(options)
+
+ merge_request.diffs(options)
+ end
+ end
+ end
+
describe "#mr_and_commit_notes" do
let!(:merge_request) { create(:merge_request) }
--
GitLab