diff options
author | Dmitriy Zaporozhets <dmitriy.zaporozhets@gmail.com> | 2015-04-30 19:47:35 +0300 |
---|---|---|
committer | Dmitriy Zaporozhets <dmitriy.zaporozhets@gmail.com> | 2015-04-30 19:47:35 +0300 |
commit | 6d22e9674456b921e0f951af10ba18505891ec10 (patch) | |
tree | 73471555eecb8de6b3fb2e520e171aaaddee4e7b | |
parent | a06354485ccbb9c31f7111327a82caf446a81a5f (diff) | |
parent | 72a7febeada2c58c98caee8bb7ce18886a7c0868 (diff) |
Merge branch 'fix-submodule-error-with-forked-project' into 'master'
Fix "Revspec not found" errors when viewing diffs in a forked project with submodules
## What does this MR do?
This MR fixes an error that occurs when viewing diffs in a forked project with submodules.
### Are there points in the code the reviewer needs to double check?
Testing this code was tricky. The only way this problem shows up is if the origin project does NOT have the submodule update commit. The introduction of gitlab-test-fork serves that purpose: it contains a submodule update not present in gitlab-test.
### Why was this MR needed?
A user would receive a 500 error when trying to view a merge request with a submodule update. #1413 has details on how to reproduce this issue.
### What are the relevant issue numbers / [Feature requests](http://feedback.gitlab.com/)?
#1413
See merge request !512
-rw-r--r-- | CHANGELOG | 1 | ||||
-rw-r--r-- | app/helpers/diff_helper.rb | 4 | ||||
-rw-r--r-- | app/helpers/submodule_helper.rb | 4 | ||||
-rw-r--r-- | app/views/projects/diffs/_file.html.haml | 2 | ||||
-rw-r--r-- | spec/controllers/merge_requests_controller_spec.rb | 20 | ||||
-rw-r--r-- | spec/factories/projects.rb | 8 | ||||
-rw-r--r-- | spec/support/test_env.rb | 53 |
7 files changed, 80 insertions, 12 deletions
diff --git a/CHANGELOG b/CHANGELOG index 305d2c2508e..3af83ddc256 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -8,6 +8,7 @@ v 7.11.0 (unreleased) - Fix "Cannot move project" error message from popping up after a successful transfer (Stan Hu) - Redirect to sign in page after signing out. - Fix "Hello @username." references not working by no longer allowing usernames to end in period. + - Fix "Revspec not found" errors when viewing diffs in a forked project with submodules (Stan Hu) - - Fix broken file browsing with relative submodule in personal projects (Stan Hu) - Add "Reply quoting selected text" shortcut key (`r`) diff --git a/app/helpers/diff_helper.rb b/app/helpers/diff_helper.rb index 4f42972a4dd..162778ade58 100644 --- a/app/helpers/diff_helper.rb +++ b/app/helpers/diff_helper.rb @@ -140,8 +140,8 @@ module DiffHelper end end - def submodule_link(blob, ref) - tree, commit = submodule_links(blob, ref) + def submodule_link(blob, ref, repository = @repository) + tree, commit = submodule_links(blob, ref, repository) commit_id = if commit.nil? blob.id[0..10] else diff --git a/app/helpers/submodule_helper.rb b/app/helpers/submodule_helper.rb index e13d4eaf101..6def7793dc3 100644 --- a/app/helpers/submodule_helper.rb +++ b/app/helpers/submodule_helper.rb @@ -2,8 +2,8 @@ module SubmoduleHelper include Gitlab::ShellAdapter # links to files listing for submodule if submodule is a project on this server - def submodule_links(submodule_item, ref = nil) - url = @repository.submodule_url_for(ref, submodule_item.path) + def submodule_links(submodule_item, ref = nil, repository = @repository) + url = repository.submodule_url_for(ref, submodule_item.path) return url, nil unless url =~ /([^\/:]+)\/([^\/]+\.git)\Z/ diff --git a/app/views/projects/diffs/_file.html.haml b/app/views/projects/diffs/_file.html.haml index 672a6635321..d4b019780f5 100644 --- a/app/views/projects/diffs/_file.html.haml +++ b/app/views/projects/diffs/_file.html.haml @@ -11,7 +11,7 @@ = view_file_btn(@commit.parent_id, diff_file, project) - elsif diff_file.diff.submodule? - submodule_item = project.repository.blob_at(@commit.id, diff_file.file_path) - = submodule_link(submodule_item, @commit.id) + = submodule_link(submodule_item, @commit.id, project.repository) - else %span - if diff_file.renamed_file diff --git a/spec/controllers/merge_requests_controller_spec.rb b/spec/controllers/merge_requests_controller_spec.rb index d6f56ed33d6..c94ef1629ae 100644 --- a/spec/controllers/merge_requests_controller_spec.rb +++ b/spec/controllers/merge_requests_controller_spec.rb @@ -78,4 +78,24 @@ describe Projects::MergeRequestsController do end end end + + context '#diffs with forked projects with submodules' do + render_views + let(:project) { create(:project) } + let(:fork_project) { create(:forked_project_with_submodules) } + let(:merge_request) { create(:merge_request_with_diffs, source_project: fork_project, source_branch: 'add-submodule-version-bump', target_branch: 'master', target_project: project) } + + before do + fork_project.build_forked_project_link(forked_to_project_id: fork_project.id, forked_from_project_id: project.id) + fork_project.save + merge_request.reload + end + + it '#diffs' do + get(:diffs, namespace_id: project.namespace.to_param, + project_id: project.to_param, id: merge_request.iid, format: 'json') + expect(response).to be_success + expect(response.body).to have_content('Subproject commit') + end + end end diff --git a/spec/factories/projects.rb b/spec/factories/projects.rb index 0899a7603fc..57fa079d753 100644 --- a/spec/factories/projects.rb +++ b/spec/factories/projects.rb @@ -76,6 +76,14 @@ FactoryGirl.define do end end + factory :forked_project_with_submodules, parent: :empty_project do + path { 'forked-gitlabhq' } + + after :create do |project| + TestEnv.copy_forked_repo_with_submodules(project) + end + end + factory :redmine_project, parent: :project do after :create do |project| project.create_redmine_service( diff --git a/spec/support/test_env.rb b/spec/support/test_env.rb index 44d70e741b2..d1844bd2b87 100644 --- a/spec/support/test_env.rb +++ b/spec/support/test_env.rb @@ -14,6 +14,10 @@ module TestEnv 'master' => '5937ac0' } + FORKED_BRANCH_SHA = BRANCH_SHA.merge({ + 'add-submodule-version-bump' => '3f547c08' + }) + # Test environment # # See gitlab.yml.example test section for paths @@ -31,6 +35,8 @@ module TestEnv # Create repository for FactoryGirl.create(:project) setup_factory_repo + # Create repository for FactoryGirl.create(:forked_project_with_submodules) + setup_forked_repo end def disable_mailer @@ -61,14 +67,26 @@ module TestEnv end def setup_factory_repo - clone_url = "https://gitlab.com/gitlab-org/#{factory_repo_name}.git" + setup_repo(factory_repo_path, factory_repo_path_bare, factory_repo_name, + BRANCH_SHA) + end + + # This repo has a submodule commit that is not present in the main test + # repository. + def setup_forked_repo + setup_repo(forked_repo_path, forked_repo_path_bare, forked_repo_name, + FORKED_BRANCH_SHA) + end + + def setup_repo(repo_path, repo_path_bare, repo_name, branch_sha) + clone_url = "https://gitlab.com/gitlab-org/#{repo_name}.git" - unless File.directory?(factory_repo_path) - system(*%W(git clone -q #{clone_url} #{factory_repo_path})) + unless File.directory?(repo_path) + system(*%W(git clone -q #{clone_url} #{repo_path})) end - Dir.chdir(factory_repo_path) do - BRANCH_SHA.each do |branch, sha| + Dir.chdir(repo_path) do + branch_sha.each do |branch, sha| # Try to reset without fetching to avoid using the network. reset = %W(git update-ref refs/heads/#{branch} #{sha}) unless system(*reset) @@ -85,7 +103,7 @@ module TestEnv end # We must copy bare repositories because we will push to them. - system(git_env, *%W(git clone -q --bare #{factory_repo_path} #{factory_repo_path_bare})) + system(git_env, *%W(git clone -q --bare #{repo_path} #{repo_path_bare})) end def copy_repo(project) @@ -100,6 +118,14 @@ module TestEnv Gitlab.config.gitlab_shell.repos_path end + def copy_forked_repo_with_submodules(project) + base_repo_path = File.expand_path(forked_repo_path_bare) + target_repo_path = File.expand_path(repos_path + "/#{project.namespace.path}/#{project.path}.git") + FileUtils.mkdir_p(target_repo_path) + FileUtils.cp_r("#{base_repo_path}/.", target_repo_path) + FileUtils.chmod_R 0755, target_repo_path + end + private def factory_repo_path @@ -114,9 +140,22 @@ module TestEnv 'gitlab-test' end + def forked_repo_path + @forked_repo_path ||= Rails.root.join('tmp', 'tests', forked_repo_name) + end + + def forked_repo_path_bare + "#{forked_repo_path}_bare" + end + + def forked_repo_name + 'gitlab-test-fork' + end + + # Prevent developer git configurations from being persisted to test # repositories def git_env - {'GIT_TEMPLATE_DIR' => ''} + { 'GIT_TEMPLATE_DIR' => '' } end end |