Fix "Revspec not found" errors when viewing diffs in a forked project with submodules

Closes #1413

7 files changed, 80 insertions, 12 deletions

diff --git a/CHANGELOG b/CHANGELOG
index 97a469513c8..a6bfedf283a 100644
--- a/CHANGELOG
+++ b/CHANGELOG
@@ -7,6 +7,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