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

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