Support renames in diff_for_path actions

app/helpers/diff_helper.rb

@@ -24,7 +24,11 @@ def diff_view
 
   def diff_options
     default_options = Commit.max_diff_options
-    default_options[:paths] = [params[:path]] if params[:path]
+
+    if action_name == 'diff_for_path'
+      default_options[:paths] = params.values_at(:old_path, :new_path)
+    end
+
     default_options.merge(ignore_whitespace_change: hide_whitespace?)
   end
 
@@ -88,7 +92,7 @@ def submodule_link(blob, ref, repository = @repository)
 
   def commit_for_diff(diff_file)
     return diff_file.content_commit if diff_file.content_commit
-    
+
     if diff_file.deleted_file
       @base_commit || @commit.parent || @commit
     else

app/models/merge_request_diff.rb

@@ -47,7 +47,7 @@ def diffs(options={})
       end
     else
       @diffs ||= {}
-      @diffs[options[:paths]] ||= load_diffs(st_diffs, options)
+      @diffs[options] ||= load_diffs(st_diffs, options)
     end
   end
 
@@ -146,8 +146,9 @@ def dump_diffs(diffs)
   def load_diffs(raw, options)
     if raw.respond_to?(:each)
       if options[:paths]
+        old_path, new_path = options[:paths]
         raw = raw.select do |diff|
-          options[:paths].include?(diff[:new_path])
+          old_path == diff[:old_path] && new_path == diff[:new_path]
         end
       end
 

app/views/projects/diffs/_content.html.haml

@@ -10,7 +10,7 @@
       .nothing-here-block This diff was suppressed by a .gitattributes entry.
     - elsif diff_file.diff_lines.length > 0
       - if diff_file.collapsed_by_default? && !expand_all_diffs?
-        - url = url_for(params.merge(action: :diff_for_path, path: diff_file.file_path, format: nil))
+        - url = url_for(params.merge(action: :diff_for_path, old_path: diff_file.old_path, new_path: diff_file.new_path))
         .nothing-here-block.diff-collapsed{data: { diff_for_path: url } }
           This diff is collapsed. Click to expand it.
       - elsif diff_view == 'parallel'

config/routes.rb

@@ -615,13 +615,13 @@
             post :retry_builds
             post :revert
             post :cherry_pick
-            get '/diffs/*path', action: :diff_for_path, constraints: { format: false }
+            get :diff_for_path
           end
         end
 
         resources :compare, only: [:index, :create] do
           collection do
-            get '/diffs/*path', action: :diff_for_path, constraints: { format: false }
+            get :diff_for_path
           end
         end
 
@@ -711,14 +711,14 @@
             post :toggle_subscription
             post :toggle_award_emoji
             post :remove_wip
-            get '/diffs/*path', action: :diff_for_path, constraints: { format: false }
+            get :diff_for_path
           end
 
           collection do
             get :branch_from
             get :branch_to
             get :update_branches
-            get '/diffs/*path', action: :diff_for_path, constraints: { format: false }
+            get :diff_for_path
           end
         end
 

spec/controllers/projects/commit_controller_spec.rb

@@ -267,7 +267,7 @@ def diff_for_path(extra_params = {})
       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)
+            diff_for_path(id: commit.id, old_path: existing_path, new_path: existing_path)
 
             expect(assigns(:diff_notes_disabled)).to be_falsey
             expect(assigns(:comments_target)).to eq(noteable_type: 'Commit',
@@ -280,12 +280,12 @@ def diff_for_path(extra_params = {})
               meth.call(diffs, diff_refs, project)
             end
 
-            diff_for_path(id: commit.id, path: existing_path)
+            diff_for_path(id: commit.id, old_path: existing_path, new_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) }
+          before { diff_for_path(id: commit.id, old_path: existing_path.succ, new_path: existing_path.succ) }
 
           it 'returns a 404' do
             expect(response).to have_http_status(404)
@@ -296,7 +296,7 @@ def diff_for_path(extra_params = {})
       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)
+          diff_for_path(id: commit.id, old_path: existing_path, new_path: existing_path)
         end
 
         it 'returns a 404' do
@@ -306,7 +306,7 @@ def diff_for_path(extra_params = {})
     end
 
     context 'when the commit does not exist' do
-      before { diff_for_path(id: commit.id.succ, path: existing_path) }
+      before { diff_for_path(id: commit.id.succ, old_path: existing_path, new_path: existing_path) }
 
       it 'returns a 404' do
         expect(response).to have_http_status(404)

spec/controllers/projects/compare_controller_spec.rb

@@ -81,7 +81,7 @@ def diff_for_path(extra_params = {})
       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)
+            diff_for_path(from: ref_from, to: ref_to, old_path: existing_path, new_path: existing_path)
 
             expect(assigns(:diff_notes_disabled)).to be_truthy
           end
@@ -92,12 +92,12 @@ def diff_for_path(extra_params = {})
               meth.call(diffs, diff_refs, project)
             end
 
-            diff_for_path(from: ref_from, to: ref_to, path: existing_path)
+            diff_for_path(from: ref_from, to: ref_to, old_path: existing_path, new_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) }
+          before { diff_for_path(from: ref_from, to: ref_to, old_path: existing_path.succ, new_path: existing_path.succ) }
 
           it 'returns a 404' do
             expect(response).to have_http_status(404)
@@ -108,7 +108,7 @@ def diff_for_path(extra_params = {})
       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)
+          diff_for_path(from: ref_from, to: ref_to, old_path: existing_path, new_path: existing_path)
         end
 
         it 'returns a 404' do
@@ -118,7 +118,7 @@ def diff_for_path(extra_params = {})
     end
 
     context 'when the from ref does not exist' do
-      before { diff_for_path(from: ref_from.succ, to: ref_to, path: existing_path) }
+      before { diff_for_path(from: ref_from.succ, to: ref_to, old_path: existing_path, new_path: existing_path) }
 
       it 'returns a 404' do
         expect(response).to have_http_status(404)
@@ -126,7 +126,7 @@ def diff_for_path(extra_params = {})
     end
 
     context 'when the to ref does not exist' do
-      before { diff_for_path(from: ref_from, to: ref_to.succ, path: existing_path) }
+      before { diff_for_path(from: ref_from, to: ref_to.succ, old_path: existing_path, new_path: existing_path) }
 
       it 'returns a 404' do
         expect(response).to have_http_status(404)

spec/controllers/projects/merge_requests_controller_spec.rb

@@ -384,7 +384,7 @@ def diff_for_path(extra_params = {})
         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)
+              diff_for_path(id: merge_request.iid, old_path: existing_path, new_path: existing_path)
 
               expect(assigns(:diff_notes_disabled)).to be_falsey
               expect(assigns(:comments_target)).to eq(noteable_type: 'MergeRequest',
@@ -397,12 +397,12 @@ def diff_for_path(extra_params = {})
                 meth.call(diffs, diff_refs, project)
               end
 
-              diff_for_path(id: merge_request.iid, path: existing_path)
+              diff_for_path(id: merge_request.iid, old_path: existing_path, new_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') }
+            before { diff_for_path(id: merge_request.iid, old_path: 'files/ruby/nopen.rb', new_path: 'files/ruby/nopen.rb') }
 
             it 'returns a 404' do
               expect(response).to have_http_status(404)
@@ -413,7 +413,7 @@ def diff_for_path(extra_params = {})
         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)
+            diff_for_path(id: merge_request.iid, old_path: existing_path, new_path: existing_path)
           end
 
           it 'returns a 404' do
@@ -423,7 +423,7 @@ def diff_for_path(extra_params = {})
       end
 
       context 'when the merge request does not exist' do
-        before { diff_for_path(id: merge_request.iid.succ, path: existing_path) }
+        before { diff_for_path(id: merge_request.iid.succ, old_path: existing_path, new_path: existing_path) }
 
         it 'returns a 404' do
           expect(response).to have_http_status(404)
@@ -435,7 +435,7 @@ def diff_for_path(extra_params = {})
 
         before do
           other_project.team << [user, :master]
-          diff_for_path(id: merge_request.iid, path: existing_path, project_id: other_project.to_param)
+          diff_for_path(id: merge_request.iid, old_path: existing_path, new_path: existing_path, project_id: other_project.to_param)
         end
 
         it 'returns a 404' do
@@ -449,7 +449,7 @@ def diff_for_path(extra_params = {})
 
       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' })
+          diff_for_path(old_path: existing_path, new_path: existing_path, merge_request: { source_branch: 'feature', target_branch: 'master' })
 
           expect(assigns(:diff_notes_disabled)).to be_truthy
         end
@@ -460,7 +460,7 @@ def diff_for_path(extra_params = {})
             meth.call(diffs, diff_refs, project)
           end
 
-          diff_for_path(path: existing_path, merge_request: { source_branch: 'feature', target_branch: 'master' })
+          diff_for_path(old_path: existing_path, new_path: existing_path, merge_request: { source_branch: 'feature', target_branch: 'master' })
         end
       end
 
@@ -471,7 +471,7 @@ def diff_for_path(extra_params = {})
 
         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' })
+            diff_for_path(old_path: existing_path, new_path: existing_path, merge_request: { source_project: other_project, source_branch: 'feature', target_branch: 'master' })
 
             expect(assigns(:diff_notes_disabled)).to be_truthy
           end
@@ -482,12 +482,12 @@ def diff_for_path(extra_params = {})
               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' })
+            diff_for_path(old_path: existing_path, new_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' }) }
+          before { diff_for_path(old_path: 'files/ruby/nopen.rb', new_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)
@@ -497,7 +497,7 @@ def diff_for_path(extra_params = {})
     end
   end
 
-  describe 'GET #commits' do
+  describe 'GET commits' do
     def go(format: 'html')
       get :commits,
           namespace_id: project.namespace.to_param,

spec/features/merge_requests/expand_collapse_diffs_spec.rb

@@ -5,7 +5,7 @@
 
   before do
     login_as :admin
-    merge_request = create(:merge_request, source_branch: 'expand-collapse-diffs', target_branch: 'master')
+    merge_request = create(:merge_request, target_branch: 'expand-collapse-diffs-start', source_branch: 'expand-collapse-diffs')
     project = merge_request.source_project
 
     # Ensure that undiffable.md is in .gitattributes
@@ -21,7 +21,12 @@ def file_container(filename)
   # Use define_method instead of let (which is memoized) so that this just works across a
   # reload.
   #
-  ['small_diff.md', 'large_diff.md', 'undiffable.md', 'too_large.md', 'too_large_image.jpg'].each do |file|
+  files = [
+    'small_diff.md', 'large_diff.md', 'large_diff_renamed.md', 'undiffable.md',
+    'too_large.md', 'too_large_image.jpg'
+  ]
+
+  files.each do |file|
     define_method(file.split('.').first) { file_container(file) }
   end
 
@@ -31,11 +36,18 @@ def file_container(filename)
       expect(small_diff).not_to have_selector('.nothing-here-block')
     end
 
-    it 'collapses larges diffs by default' do
+    it 'collapses large diffs by default' do
       expect(large_diff).not_to have_selector('.code')
       expect(large_diff).to have_selector('.nothing-here-block')
     end
 
+    it 'collapses large diffs for renamed files by default' do
+      expect(large_diff_renamed).not_to have_selector('.code')
+      expect(large_diff_renamed).to have_selector('.nothing-here-block')
+      expect(large_diff_renamed).to have_selector('.file-title .deletion')
+      expect(large_diff_renamed).to have_selector('.file-title .addition')
+    end
+
     it 'shows non-renderable diffs as such immediately, regardless of their size' do
       expect(undiffable).not_to have_selector('.code')
       expect(undiffable).to have_selector('.nothing-here-block')
@@ -53,6 +65,25 @@ def file_container(filename)
       expect(too_large_image).to have_selector('.image')
     end
 
+    context 'expanding a diff for a renamed file' do
+      before do
+        large_diff_renamed.find('.nothing-here-block').click
+        wait_for_ajax
+      end
+
+      it 'shows the old content' do
+        old_line = large_diff_renamed.find('.line_content.old')
+
+        expect(old_line).to have_content('four copies')
+      end
+
+      it 'shows the new content' do
+        new_line = large_diff_renamed.find('.line_content.new', match: :prefer_exact)
+
+        expect(new_line).to have_content('six copies')
+      end
+    end
+
     context 'expanding a large diff' do
       before do
         click_link('large_diff.md')

spec/models/merge_request_diff_spec.rb

@@ -30,10 +30,10 @@
       end
 
       context 'when the :paths option is set' do
-        let(:diffs) { mr_diff.diffs(paths: ['.gitignore', 'files/ruby/popen.rb', 'files/ruby/string.rb']) }
+        let(:diffs) { mr_diff.diffs(paths: ['files/ruby/popen.rb', 'files/ruby/popen.rb']) }
 
-        it 'only returns diffs that match the paths given' do
-          expect(diffs.map(&:new_path)).to contain_exactly('.gitignore', 'files/ruby/popen.rb')
+        it 'only returns diffs that match the (old path, new path) given' do
+          expect(diffs.map(&:new_path)).to contain_exactly('files/ruby/popen.rb')
         end
 
         it 'uses the diffs from the DB' do

spec/support/test_env.rb

@@ -5,19 +5,21 @@ module TestEnv
 
   # When developing the seed repository, comment out the branch you will modify.
   BRANCH_SHA = {
-    'empty-branch'     => '7efb185',
-    'flatten-dir'      => 'e56497b',
-    'feature'          => '0b4bc9a',
-    'feature_conflict' => 'bb5206f',
-    'fix'              => '48f0be4',
-    'improve/awesome'  => '5937ac0',
-    'markdown'         => '0ed8c6c',
-    'lfs'              => 'be93687',
-    'master'           => '5937ac0',
-    "'test'"           => 'e56497b',
-    'orphaned-branch'  => '45127a9',
-    'binary-encoding'  => '7b1cf43',
-    'gitattributes'    => '5a62481',
+    'empty-branch'                => '7efb185',
+    'flatten-dir'                 => 'e56497b',
+    'feature'                     => '0b4bc9a',
+    'feature_conflict'            => 'bb5206f',
+    'fix'                         => '48f0be4',
+    'improve/awesome'             => '5937ac0',
+    'markdown'                    => '0ed8c6c',
+    'lfs'                         => 'be93687',
+    'master'                      => '5937ac0',
+    "'test'"                      => 'e56497b',
+    'orphaned-branch'             => '45127a9',
+    'binary-encoding'             => '7b1cf43',
+    'gitattributes'               => '5a62481',
+    'expand-collapse-diffs-start' => '65b04e4',
+    'expand-collapse-diffs'       => '865e6d5'
   }
 
   # gitlab-test-fork is a fork of gitlab-fork, but we don't necessarily