diff --git a/app/controllers/commit_controller.rb b/app/controllers/commit_controller.rb index 97998352255ff6e1aa47b64cd2e1031bcf9753e1..d671e9f9e9e1721fdf59ee1631c814f7fb92d626 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 8e180c9baae7cd78129755f84ceced37d4bc54cc..362962707fd800e41e4ec8ecd93f739c209d6026 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 5efb20cea3b447417f3264e7253a9e178eceef75..200c915a3354cd8334634b6562a0243e2e941bb6 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 0766e5baa7275dfa177dec0388f63d45be28f0d0..8039813ad1ce4c2de15bf3cd39377b0e86f7735c 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 ce1c3d023f538a394934c9241e94fb2216418688..0000000000000000000000000000000000000000 --- 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 26753a1465f7b159be8a1cf7d5e1787f2ca5ac9e..8f7826e0c8db9841abdcdab6a49a196850a4db7e 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 7685090311a6b26b59303888098483d3ecf705b1..0807454c4b02d9298a13f90959c3446096e0fe92 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 8708469cc5deb1c1f47730f5841c529b5c554870..a5275650d861d652239fc4329d87aafb109378f5 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 3549b8362bb4042339a5f4412d56b70bb87b7a9d..8f8bef42befd6c13ced10f6345d7d40cb7bec968 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 98cf7e812c90b138debe0d8c4a80e8b08d7db6f6..4669306f923619e9cb5c28a5c213ce3c2bfb28c1 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 0000000000000000000000000000000000000000..5aef4c676eeb9d3b04342f5a06507e93857efda5 --- /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 0000000000000000000000000000000000000000..7cd1285fab497f02779d0eb19b2bd7da5fb6f447 --- /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 7c33f0ecc8b546ec7c4b48d8520fb8e9790dcba6..e0b472afea531fed455c42c7d72c93eec91d23ce 100644 --- a/spec/factories.rb +++ b/spec/factories.rb @@ -63,7 +63,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 dc687d2a7ac9f0581cae85d37b8b146c8dd9db8b..25db2f91d4d89541fe414b64628adf715d4fe963 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