Merge pull request #2067 from riyad/diff-and-patch-for-commits-and-merge-requests

Diff and patch for commits and merge requests

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

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

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

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

app/views/commit/show.patch.erb

@@ -1 +0,0 @@
-<%= @commit.to_patch %>

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
+      .left.btn-group
+        %a.btn.small.grouped.dropdown-toggle{ data: {toggle: :dropdown} }
           %i.icon-download-alt
-        Get Patch
+          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

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

@@ -3,6 +3,8 @@
 - elsif @merge_request.broken_diffs?
   %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

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 
+        .left.btn-group
+          %a.btn.grouped.dropdown-toggle{ data: {toggle: :dropdown} }
             %i.icon-download-alt
-          Get Patch  
+            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"
 

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

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

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('&amp;')
+        expect(response.body).to_not include('&gt;')
+        expect(response.body).to_not include('&lt;')
+        expect(response.body).to_not include('&quot;')
+      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

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('&amp;')
+        expect(response.body).to_not include('&gt;')
+        expect(response.body).to_not include('&lt;')
+        expect(response.body).to_not include('&quot;')
+      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

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

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