From b754a6678d0f3199b7e0d693989e68002c0d7d6a Mon Sep 17 00:00:00 2001 From: Stan Hu Date: Sat, 27 Aug 2016 20:59:48 -0700 Subject: [PATCH 1/2] Fix Error 500 when creating a merge request that contains an image that was deleted and added Steps to reproduce: 1. Start with a repo with an image 2. Add a commit to delete the image 3. Add another commit to replace the image with another image In a diff comparison, we really just compare about what the image was before the diff, not the direct parent of the last commit. This MR fixes that. Closes #3893, gitlab-org/gitlab-ee#678 --- CHANGELOG | 1 + app/helpers/diff_helper.rb | 6 ++++++ app/views/projects/diffs/_content.html.haml | 2 +- app/views/projects/diffs/_diffs.html.haml | 3 ++- app/views/projects/diffs/_file.html.haml | 2 +- lib/gitlab/diff/file.rb | 13 ++++++++----- spec/features/merge_requests/create_new_mr_spec.rb | 10 ++++++++++ spec/support/test_env.rb | 1 + 8 files changed, 30 insertions(+), 8 deletions(-) diff --git a/CHANGELOG b/CHANGELOG index d69168985cd3..88277026fbce 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -29,6 +29,7 @@ v 8.12.0 (unreleased) - Enable pipeline events by default !6278 - Move parsing of sidekiq ps into helper !6245 (pascalbetz) - Expose `sha` and `merge_commit_sha` in merge request API (Ben Boeckel) + - Fix Error 500 when creating a merge request that contains an image that was deleted and added - Set path for all JavaScript cookies to honor GitLab's subdirectory setting !5627 (Mike Greiling) - Fix blame table layout width - Fix bug where pagination is still displayed despite all todos marked as done (ClemMakesApps) diff --git a/app/helpers/diff_helper.rb b/app/helpers/diff_helper.rb index 0725c3f4c56c..e505ffb4f4b1 100644 --- a/app/helpers/diff_helper.rb +++ b/app/helpers/diff_helper.rb @@ -109,6 +109,12 @@ def commit_for_diff(diff_file) end end + def old_commit_for_diff(diff_file) + return diff_file.old_content_commit if diff_file.old_content_commit + + @base_commit + end + def diff_file_html_data(project, diff_file_path, diff_commit_id) { blob_diff_path: namespace_project_blob_diff_path(project.namespace, project, diff --git a/app/views/projects/diffs/_content.html.haml b/app/views/projects/diffs/_content.html.haml index d37961c4e40d..4ef02b7353f3 100644 --- a/app/views/projects/diffs/_content.html.haml +++ b/app/views/projects/diffs/_content.html.haml @@ -23,7 +23,7 @@ - elsif diff_file.renamed_file .nothing-here-block File moved - elsif blob.image? - - old_blob = diff_file.old_blob(diff_commit) + - old_blob = diff_file.old_blob(old_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/_diffs.html.haml b/app/views/projects/diffs/_diffs.html.haml index 62aff36aadd7..ad9348170ce5 100644 --- a/app/views/projects/diffs/_diffs.html.haml +++ b/app/views/projects/diffs/_diffs.html.haml @@ -25,9 +25,10 @@ .files{data: {can_create_note: (!@diff_notes_disabled && can?(current_user, :create_note, diffs.project))}} - diff_files.each_with_index do |diff_file, index| - diff_commit = commit_for_diff(diff_file) + - old_diff_commit = old_commit_for_diff(diff_file) - blob = diff_file.blob(diff_commit) - next unless blob - blob.load_all_data!(diffs.project.repository) unless blob.only_display_raw? = render 'projects/diffs/file', index: index, project: diffs.project, - diff_file: diff_file, diff_commit: diff_commit, blob: blob + diff_file: diff_file, diff_commit: diff_commit, old_diff_commit: old_diff_commit, blob: blob diff --git a/app/views/projects/diffs/_file.html.haml b/app/views/projects/diffs/_file.html.haml index ad2eb3e504f5..7b6698958972 100644 --- a/app/views/projects/diffs/_file.html.haml +++ b/app/views/projects/diffs/_file.html.haml @@ -16,4 +16,4 @@ = view_file_btn(diff_commit.id, diff_file.new_path, project) - = render 'projects/diffs/content', diff_file: diff_file, diff_commit: diff_commit, blob: blob, project: project + = render 'projects/diffs/content', diff_file: diff_file, diff_commit: diff_commit, old_diff_commit: old_diff_commit, blob: blob, project: project diff --git a/lib/gitlab/diff/file.rb b/lib/gitlab/diff/file.rb index e47df508ca29..5aeb48a0a2d8 100644 --- a/lib/gitlab/diff/file.rb +++ b/lib/gitlab/diff/file.rb @@ -55,6 +55,12 @@ def content_commit repository.commit(deleted_file ? old_ref : new_ref) end + def old_content_commit + return unless diff_refs + + repository.commit(old_ref) + end + def old_ref diff_refs.try(:base_sha) end @@ -111,13 +117,10 @@ def removed_lines diff_lines.count(&:removed?) end - def old_blob(commit = content_commit) + def old_blob(commit = old_content_commit) return unless commit - parent_id = commit.parent_id - return unless parent_id - - repository.blob_at(parent_id, old_path) + repository.blob_at(commit.id, old_path) end def blob(commit = content_commit) diff --git a/spec/features/merge_requests/create_new_mr_spec.rb b/spec/features/merge_requests/create_new_mr_spec.rb index b963d1305b53..524f3b77a232 100644 --- a/spec/features/merge_requests/create_new_mr_spec.rb +++ b/spec/features/merge_requests/create_new_mr_spec.rb @@ -59,4 +59,14 @@ expect(page).to have_css('a.btn.active', text: 'Side-by-side') end end + + context 'when a branch contains commits that both delete and add the same image' do + it 'renders the diff successfully' do + visit new_namespace_project_merge_request_path(project.namespace, project, merge_request: { target_branch: 'master', source_branch: 'deleted-image-test' }) + + click_link "Changes" + + expect(page).to have_content "6049019_460s.jpg" + end + end end diff --git a/spec/support/test_env.rb b/spec/support/test_env.rb index 0097dbf8fadc..136aed7c342d 100644 --- a/spec/support/test_env.rb +++ b/spec/support/test_env.rb @@ -31,6 +31,7 @@ module TestEnv 'conflict-missing-side' => 'eb227b3', 'conflict-non-utf8' => 'd0a293c', 'conflict-too-large' => '39fa04f', + 'deleted-image-test' => '6c17798' } # gitlab-test-fork is a fork of gitlab-fork, but we don't necessarily -- GitLab From 6137d411bb0319c61e1fd6274c2ae8b874eb5ce7 Mon Sep 17 00:00:00 2001 From: Stan Hu Date: Mon, 12 Sep 2016 20:15:34 -0700 Subject: [PATCH 2/2] Add specs for Diff::File --- spec/lib/gitlab/diff/file_spec.rb | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/spec/lib/gitlab/diff/file_spec.rb b/spec/lib/gitlab/diff/file_spec.rb index 0650cb291e5f..27b052111a8f 100644 --- a/spec/lib/gitlab/diff/file_spec.rb +++ b/spec/lib/gitlab/diff/file_spec.rb @@ -46,4 +46,20 @@ expect(diff_file.collapsed?).to eq(false) end end + + describe '#old_blob' do + it 'returns blob of commit of base commit' do + old_data = diff_file.old_blob.data + + expect(old_data).to include('raise "System commands must be given as an array of strings"') + end + end + + describe '#blob' do + it 'returns blob of new commit' do + data = diff_file.blob.data + + expect(data).to include('raise RuntimeError, "System commands must be given as an array of strings"') + end + end end -- GitLab