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

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`)

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

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/
 

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

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

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(

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