diff options
| -rw-r--r-- | app/controllers/commit_controller.rb | 3 | ||||
| -rw-r--r-- | app/controllers/merge_requests_controller.rb | 12 | ||||
| -rw-r--r-- | app/models/commit.rb | 15 | ||||
| -rw-r--r-- | app/models/merge_request.rb | 26 | ||||
| -rw-r--r-- | app/views/commit/show.patch.erb | 1 | ||||
| -rw-r--r-- | app/views/commits/_commit_box.html.haml | 11 | ||||
| -rw-r--r-- | app/views/merge_requests/show/_diffs.html.haml | 6 | ||||
| -rw-r--r-- | app/views/merge_requests/show/_mr_title.html.haml | 11 | ||||
| -rw-r--r-- | config/initializers/mime_types.rb | 3 | ||||
| -rw-r--r-- | config/routes.rb | 3 | ||||
| -rw-r--r-- | spec/controllers/commit_controller_spec.rb | 74 | ||||
| -rw-r--r-- | spec/controllers/merge_requests_controller_spec.rb | 84 | ||||
| -rw-r--r-- | spec/factories.rb | 15 | ||||
| -rw-r--r-- | spec/routing/project_routing_spec.rb | 11 |
14 files changed, 238 insertions, 37 deletions
diff --git a/app/controllers/commit_controller.rb b/app/controllers/commit_controller.rb index 97998352255..d671e9f9e9e 100644 --- a/app/controllers/commit_controller.rb +++ b/app/controllers/commit_controller.rb @@ -26,7 +26,8 @@ class CommitController < ProjectResourceController end end - format.patch + format.diff { render text: @commit.to_diff } + format.patch { render text: @commit.to_patch } end end end diff --git a/app/controllers/merge_requests_controller.rb b/app/controllers/merge_requests_controller.rb index 8e180c9baae..362962707fd 100644 --- a/app/controllers/merge_requests_controller.rb +++ b/app/controllers/merge_requests_controller.rb @@ -1,7 +1,7 @@ class MergeRequestsController < ProjectResourceController before_filter :module_enabled - before_filter :merge_request, only: [:edit, :update, :destroy, :show, :commits, :diffs, :automerge, :automerge_check, :raw] - before_filter :validates_merge_request, only: [:show, :diffs, :raw] + before_filter :merge_request, only: [:edit, :update, :destroy, :show, :commits, :diffs, :automerge, :automerge_check] + before_filter :validates_merge_request, only: [:show, :diffs] before_filter :define_show_vars, only: [:show, :diffs] # Allow read any merge_request @@ -16,7 +16,6 @@ class MergeRequestsController < ProjectResourceController # Allow destroy merge_request before_filter :authorize_admin_merge_request!, only: [:destroy] - def index @merge_requests = MergeRequestsLoadContext.new(project, current_user, params).execute end @@ -25,11 +24,10 @@ class MergeRequestsController < ProjectResourceController respond_to do |format| format.html format.js - end - end - def raw - send_file @merge_request.to_raw + format.diff { render text: @merge_request.to_diff } + format.patch { render text: @merge_request.to_patch } + end end def diffs diff --git a/app/models/commit.rb b/app/models/commit.rb index 5efb20cea3b..200c915a335 100644 --- a/app/models/commit.rb +++ b/app/models/commit.rb @@ -150,4 +150,19 @@ class Commit def parents_count parents && parents.count || 0 end + + # Shows the diff between the commit's parent and the commit. + # + # Cuts out the header and stats from #to_patch and returns only the diff. + def to_diff + # see Grit::Commit#show + patch = to_patch + + # discard lines before the diff + lines = patch.split("\n") + while !lines.first.start_with?("diff --git") do + lines.shift + end + lines.join("\n") + end end diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb index 0766e5baa72..8039813ad1c 100644 --- a/app/models/merge_request.rb +++ b/app/models/merge_request.rb @@ -202,20 +202,22 @@ class MergeRequest < ActiveRecord::Base false end - def to_raw - FileUtils.mkdir_p(Rails.root.join("tmp", "patches")) - patch_path = Rails.root.join("tmp", "patches", "merge_request_#{self.id}.patch") - - from = commits.last.id - to = source_branch - - project.repo.git.run('', "format-patch" , " > #{patch_path.to_s}", {}, ["#{from}..#{to}", "--stdout"]) - - patch_path - end - def mr_and_commit_notes commit_ids = commits.map(&:id) Note.where("(noteable_type = 'MergeRequest' AND noteable_id = :mr_id) OR (noteable_type = 'Commit' AND noteable_id IN (:commit_ids))", mr_id: id, commit_ids: commit_ids) end + + # Returns the raw diff for this merge request + # + # see "git diff" + def to_diff + project.repo.git.native(:diff, {timeout: 30, raise: true}, "#{target_branch}...#{source_branch}") + end + + # Returns the commit as a series of email patches. + # + # see "git format-patch" + def to_patch + project.repo.git.format_patch({timeout: 30, raise: true, stdout: true}, "#{target_branch}..#{source_branch}") + end end diff --git a/app/views/commit/show.patch.erb b/app/views/commit/show.patch.erb deleted file mode 100644 index ce1c3d023f5..00000000000 --- a/app/views/commit/show.patch.erb +++ /dev/null @@ -1 +0,0 @@ -<%= @commit.to_patch %> diff --git a/app/views/commits/_commit_box.html.haml b/app/views/commits/_commit_box.html.haml index 26753a1465f..8f7826e0c8d 100644 --- a/app/views/commits/_commit_box.html.haml +++ b/app/views/commits/_commit_box.html.haml @@ -5,9 +5,14 @@ %span.btn.disabled.grouped %i.icon-comment = @notes_count - = link_to project_commit_path(@project, @commit, format: :patch), class: "btn small grouped" do - %i.icon-download-alt - Get Patch + .left.btn-group + %a.btn.small.grouped.dropdown-toggle{ data: {toggle: :dropdown} } + %i.icon-download-alt + Download as + %span.caret + %ul.dropdown-menu + %li= link_to "Email Patches", project_commit_path(@project, @commit, format: :patch) + %li= link_to "Plain Diff", project_commit_path(@project, @commit, format: :diff) = link_to project_tree_path(@project, @commit), class: "browse-button primary grouped" do %strong Browse Code ยป %h3.commit-title.page_title diff --git a/app/views/merge_requests/show/_diffs.html.haml b/app/views/merge_requests/show/_diffs.html.haml index 7685090311a..0807454c4b0 100644 --- a/app/views/merge_requests/show/_diffs.html.haml +++ b/app/views/merge_requests/show/_diffs.html.haml @@ -1,8 +1,10 @@ - if @merge_request.valid_diffs? = render "commits/diffs", diffs: @diffs - elsif @merge_request.broken_diffs? - %h4.nothing_here_message + %h4.nothing_here_message Can't load diff. - You can #{link_to "download MR patch", raw_project_merge_request_path(@project, @merge_request), class: "vlink"} instead. + You can + = link_to "download it", project_merge_request_path(@project, @merge_request), format: :diff, class: "vlink" + instead. - else %h4.nothing_here_message Nothing to merge diff --git a/app/views/merge_requests/show/_mr_title.html.haml b/app/views/merge_requests/show/_mr_title.html.haml index 8708469cc5d..a5275650d86 100644 --- a/app/views/merge_requests/show/_mr_title.html.haml +++ b/app/views/merge_requests/show/_mr_title.html.haml @@ -13,9 +13,14 @@ = "MERGED" - if can?(current_user, :modify_merge_request, @merge_request) - if @merge_request.open? - = link_to raw_project_merge_request_path(@project, @merge_request), class: "btn grouped" do - %i.icon-download-alt - Get Patch + .left.btn-group + %a.btn.grouped.dropdown-toggle{ data: {toggle: :dropdown} } + %i.icon-download-alt + Download as + %span.caret + %ul.dropdown-menu + %li= link_to "Email Patches", project_merge_request_path(@project, @merge_request, format: :patch) + %li= link_to "Plain Diff", project_merge_request_path(@project, @merge_request, format: :diff) = link_to 'Close', project_merge_request_path(@project, @merge_request, merge_request: {closed: true }, status_only: true), method: :put, class: "btn grouped danger", title: "Close merge request" diff --git a/config/initializers/mime_types.rb b/config/initializers/mime_types.rb index 3549b8362bb..8f8bef42bef 100644 --- a/config/initializers/mime_types.rb +++ b/config/initializers/mime_types.rb @@ -4,4 +4,5 @@ # Mime::Type.register "text/richtext", :rtf # Mime::Type.register_alias "text/html", :iphone -Mime::Type.register_alias 'text/plain', :patch +Mime::Type.register_alias "text/plain", :diff +Mime::Type.register_alias "text/plain", :patch diff --git a/config/routes.rb b/config/routes.rb index a7006ec0913..9c58ce17fc2 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -159,12 +159,11 @@ Gitlab::Application.routes.draw do end end - resources :merge_requests do + resources :merge_requests, constraints: {id: /\d+/} do member do get :diffs get :automerge get :automerge_check - get :raw end collection do diff --git a/spec/controllers/commit_controller_spec.rb b/spec/controllers/commit_controller_spec.rb new file mode 100644 index 00000000000..5aef4c676ee --- /dev/null +++ b/spec/controllers/commit_controller_spec.rb @@ -0,0 +1,74 @@ +require 'spec_helper' + +describe CommitController do + let(:project) { create(:project) } + let(:user) { create(:user) } + let(:commit) { project.last_commit_for("master") } + + before do + sign_in(user) + + project.add_access(user, :read, :admin) + end + + describe "#show" do + shared_examples "export as" do |format| + it "should generally work" do + get :show, project_id: project.code, id: commit.id, format: format + + expect(response).to be_success + end + + it "should generate it" do + Commit.any_instance.should_receive(:"to_#{format}") + + get :show, project_id: project.code, id: commit.id, format: format + end + + it "should render it" do + get :show, project_id: project.code, id: commit.id, format: format + + expect(response.body).to eq(commit.send(:"to_#{format}")) + end + + it "should not escape Html" do + Commit.any_instance.stub(:"to_#{format}").and_return('HTML entities &<>" ') + + get :show, project_id: project.code, id: commit.id, format: format + + expect(response.body).to_not include('&') + expect(response.body).to_not include('>') + expect(response.body).to_not include('<') + expect(response.body).to_not 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, project_id: project.code, id: commit.id, format: format + + expect(response.body).to start_with("diff --git") + 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, project_id: project.code, id: commit.id, format: format + + expect(response.body).to start_with("From #{commit.id}") + end + + it "should contain a git diff" do + get :show, project_id: project.code, id: commit.id, format: format + + expect(response.body).to match(/^diff --git/) + end + end + end +end diff --git a/spec/controllers/merge_requests_controller_spec.rb b/spec/controllers/merge_requests_controller_spec.rb new file mode 100644 index 00000000000..7cd1285fab4 --- /dev/null +++ b/spec/controllers/merge_requests_controller_spec.rb @@ -0,0 +1,84 @@ +require 'spec_helper' + +describe MergeRequestsController do + let(:project) { create(:project) } + let(:user) { create(:user) } + let(:merge_request) { create(:merge_request_with_diffs, project: project) } + + before do + sign_in(user) + + project.add_access(user, :read, :admin) + end + + describe "#show" do + shared_examples "export as" do |format| + it "should generally work" do + get :show, project_id: project.code, id: merge_request.id, format: format + + expect(response).to be_success + end + + it "should generate it" do + MergeRequest.any_instance.should_receive(:"to_#{format}") + + get :show, project_id: project.code, id: merge_request.id, format: format + end + + it "should render it" do + get :show, project_id: project.code, id: merge_request.id, format: format + + expect(response.body).to eq(merge_request.send(:"to_#{format}")) + end + + it "should not escape Html" do + MergeRequest.any_instance.stub(:"to_#{format}").and_return('HTML entities &<>" ') + + get :show, project_id: project.code, id: merge_request.id, format: format + + expect(response.body).to_not include('&') + expect(response.body).to_not include('>') + expect(response.body).to_not include('<') + expect(response.body).to_not 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, project_id: project.code, id: merge_request.id, format: format + + expect(response.body).to start_with("diff --git") + end + end + + describe "as patch" do + include_examples "export as", :patch + let(:format) { :patch } + + it "should really be a git email patch with commit" do + get :show, project_id: project.code, id: merge_request.id, format: format + + expect(response.body[0..100]).to start_with("From #{merge_request.commits.last.id}") + end + + it "should contain as many patches as there are commits" do + get :show, project_id: project.code, id: merge_request.id, format: format + + patch_count = merge_request.commits.count + merge_request.commits.each_with_index do |commit, patch_num| + expect(response.body).to match(/^From #{commit.id}/) + expect(response.body).to match(/^Subject: \[PATCH #{patch_num}\/#{patch_count}\]/) + end + end + + it "should contain git diffs" do + get :show, project_id: project.code, id: merge_request.id, format: format + + expect(response.body).to match(/^diff --git/) + end + end + end +end diff --git a/spec/factories.rb b/spec/factories.rb index c673606b984..a26a77dd860 100644 --- a/spec/factories.rb +++ b/spec/factories.rb @@ -70,7 +70,22 @@ FactoryGirl.define do closed true end + # pick 3 commits "at random" (from bcf03b5d~3 to bcf03b5d) + trait :with_diffs do + target_branch "bcf03b5d~3" + source_branch "bcf03b5d" + st_commits do + [Commit.new(project.repo.commit('bcf03b5d')), + Commit.new(project.repo.commit('bcf03b5d~1')), + Commit.new(project.repo.commit('bcf03b5d~2'))] + end + st_diffs do + project.repo.diff("bcf03b5d~3", "bcf03b5d") + end + end + factory :closed_merge_request, traits: [:closed] + factory :merge_request_with_diffs, traits: [:with_diffs] end factory :note do diff --git a/spec/routing/project_routing_spec.rb b/spec/routing/project_routing_spec.rb index dc687d2a7ac..25db2f91d4d 100644 --- a/spec/routing/project_routing_spec.rb +++ b/spec/routing/project_routing_spec.rb @@ -208,7 +208,6 @@ end # diffs_project_merge_request GET /:project_id/merge_requests/:id/diffs(.:format) merge_requests#diffs # automerge_project_merge_request GET /:project_id/merge_requests/:id/automerge(.:format) merge_requests#automerge # automerge_check_project_merge_request GET /:project_id/merge_requests/:id/automerge_check(.:format) merge_requests#automerge_check -# raw_project_merge_request GET /:project_id/merge_requests/:id/raw(.:format) merge_requests#raw # branch_from_project_merge_requests GET /:project_id/merge_requests/branch_from(.:format) merge_requests#branch_from # branch_to_project_merge_requests GET /:project_id/merge_requests/branch_to(.:format) merge_requests#branch_to # project_merge_requests GET /:project_id/merge_requests(.:format) merge_requests#index @@ -231,10 +230,6 @@ describe MergeRequestsController, "routing" do get("/gitlabhq/merge_requests/1/automerge_check").should route_to('merge_requests#automerge_check', project_id: 'gitlabhq', id: '1') end - it "to #raw" do - get("/gitlabhq/merge_requests/1/raw").should route_to('merge_requests#raw', project_id: 'gitlabhq', id: '1') - end - it "to #branch_from" do get("/gitlabhq/merge_requests/branch_from").should route_to('merge_requests#branch_from', project_id: 'gitlabhq') end @@ -243,6 +238,11 @@ describe MergeRequestsController, "routing" do get("/gitlabhq/merge_requests/branch_to").should route_to('merge_requests#branch_to', project_id: 'gitlabhq') end + it "to #show" do + get("/gitlabhq/merge_requests/1.diff").should route_to('merge_requests#show', project_id: 'gitlabhq', id: '1', format: 'diff') + get("/gitlabhq/merge_requests/1.patch").should route_to('merge_requests#show', project_id: 'gitlabhq', id: '1', format: 'patch') + end + it_behaves_like "RESTful project resources" do let(:controller) { 'merge_requests' } end @@ -285,6 +285,7 @@ end describe CommitController, "routing" do it "to #show" do get("/gitlabhq/commit/4246fb").should route_to('commit#show', project_id: 'gitlabhq', id: '4246fb') + get("/gitlabhq/commit/4246fb.diff").should route_to('commit#show', project_id: 'gitlabhq', id: '4246fb', format: 'diff') get("/gitlabhq/commit/4246fb.patch").should route_to('commit#show', project_id: 'gitlabhq', id: '4246fb', format: 'patch') get("/gitlabhq/commit/4246fbd13872934f72a8fd0d6fb1317b47b59cb5").should route_to('commit#show', project_id: 'gitlabhq', id: '4246fbd13872934f72a8fd0d6fb1317b47b59cb5') end |