Merge branch '37356-relative-submodule-link' into 'master'

Fix git submodule link for subgroup projects with relative path

Closes #37356

See merge request gitlab-org/gitlab-ce!21154

app/helpers/submodule_helper.rb

@@ -64,8 +64,7 @@ module SubmoduleHelper
   end
 
   def relative_self_url?(url)
-    # (./)?(../repo.git) || (./)?(../../project/repo.git) )
-    url =~ %r{\A((\./)?(\.\./))(?!(\.\.)|(.*/)).*(\.git)?\z} || url =~ %r{\A((\./)?(\.\./){2})(?!(\.\.))([^/]*)/(?!(\.\.)|(.*/)).*(\.git)?\z}
+    url.start_with?('../', './')
   end
 
   def standard_links(host, namespace, project, commit)
@@ -73,25 +72,29 @@ module SubmoduleHelper
     [base, [base, '/tree/', commit].join('')]
   end
 
-  def relative_self_links(url, commit, project)
-    url.rstrip!
-    # Map relative links to a namespace and project
-    # For example:
-    # ../bar.git -> same namespace, repo bar
-    # ../foo/bar.git -> namespace foo, repo bar
-    # ../../foo/bar/baz.git -> namespace bar, repo baz
-    components = url.split('/')
-    base = components.pop.gsub(/.git$/, '')
-    namespace = components.pop.gsub(/^\.\.$/, '')
+  def relative_self_links(relative_path, commit, project)
+    relative_path.rstrip!
+    absolute_project_path = "/" + project.full_path
 
-    if namespace.empty?
-      namespace = project.namespace.full_path
+    # Resolve `relative_path` to target path
+    # Assuming `absolute_project_path` is `/g1/p1`:
+    # ../p2.git -> /g1/p2
+    # ../g2/p3.git -> /g1/g2/p3
+    # ../../g3/g4/p4.git -> /g3/g4/p4
+    submodule_project_path = File.absolute_path(relative_path, absolute_project_path)
+    target_namespace_path = File.dirname(submodule_project_path)
+
+    if target_namespace_path == '/' || target_namespace_path.start_with?(absolute_project_path)
+      return [nil, nil]
     end
 
+    target_namespace_path.sub!(%r{^/}, '')
+    submodule_base = File.basename(submodule_project_path, '.git')
+
     begin
       [
-        namespace_project_path(namespace, base),
-        namespace_project_tree_path(namespace, base, commit)
+        namespace_project_path(target_namespace_path, submodule_base),
+        namespace_project_tree_path(target_namespace_path, submodule_base, commit)
       ]
     rescue ActionController::UrlGenerationError
       [nil, nil]

changelogs/unreleased/37356-relative-submodule-link.yml

+---
+title: Fix git submodule link for subgroup projects with relative path
+merge_request: 21154
+author:
+type: fixed

spec/helpers/submodule_helper_spec.rb

@@ -162,42 +162,77 @@ describe SubmoduleHelper do
     end
 
     context 'submodules with relative links' do
-      let(:group) { create(:group, name: "Master Project", path: "master-project") }
+      let(:group) { create(:group, name: "top group", path: "top-group") }
       let(:project) { create(:project, group: group) }
-      let(:commit_id) { sample_commit[:id] }
+      let(:repo) { double(:repo, project: project) }
+
+      def expect_relative_link_to_resolve_to(relative_path, expected_path)
+        allow(repo).to receive(:submodule_url_for).and_return(relative_path)
+
+        result = submodule_links(submodule_item)
 
-      it 'one level down' do
-        result = relative_self_links('../test.git', commit_id, project)
-        expect(result).to eq(["/#{group.path}/test", "/#{group.path}/test/tree/#{commit_id}"])
+        expect(result).to eq([expected_path, "#{expected_path}/tree/#{submodule_item.id}"])
       end
 
-      it 'with trailing whitespace' do
-        result = relative_self_links('../test.git ', commit_id, project)
-        expect(result).to eq(["/#{group.path}/test", "/#{group.path}/test/tree/#{commit_id}"])
+      it 'handles project under same group' do
+        expect_relative_link_to_resolve_to('../test.git', "/#{group.path}/test")
       end
 
-      it 'two levels down' do
-        result = relative_self_links('../../test.git', commit_id, project)
-        expect(result).to eq(["/#{group.path}/test", "/#{group.path}/test/tree/#{commit_id}"])
+      it 'handles trailing whitespace' do
+        expect_relative_link_to_resolve_to('../test.git ', "/#{group.path}/test")
       end
 
-      it 'one level down with namespace and repo' do
-        result = relative_self_links('../foobar/test.git', commit_id, project)
-        expect(result).to eq(["/foobar/test", "/foobar/test/tree/#{commit_id}"])
+      it 'handles project under another top group' do
+        expect_relative_link_to_resolve_to('../../baz/test.git ', "/baz/test")
       end
 
-      it 'two levels down with namespace and repo' do
-        result = relative_self_links('../foobar/baz/test.git', commit_id, project)
-        expect(result).to eq(["/baz/test", "/baz/test/tree/#{commit_id}"])
+      context 'repo path resolves to be located at root (namespace absent)' do
+        it 'returns nil' do
+          allow(repo).to receive(:submodule_url_for).and_return('../../test.git')
+
+          result = submodule_links(submodule_item)
+
+          expect(result).to eq([nil, nil])
+        end
+      end
+
+      context 'repo path resolves to be located underneath current project path' do
+        it 'returns nil because it is not possible to have repo nested under another repo' do
+          allow(repo).to receive(:submodule_url_for).and_return('./test.git')
+
+          result = submodule_links(submodule_item)
+
+          expect(result).to eq([nil, nil])
+        end
+      end
+
+      context 'subgroup' do
+        let(:sub_group) { create(:group, parent: group, name: "sub group", path: "sub-group") }
+        let(:sub_project) { create(:project, group: sub_group) }
+
+        context 'project in sub group' do
+          let(:project) { sub_project }
+
+          it "handles referencing ancestor group's project" do
+            expect_relative_link_to_resolve_to('../../../top-group/test.git', "/#{group.path}/test")
+          end
+        end
+
+        it "handles referencing descendent group's project" do
+          expect_relative_link_to_resolve_to('../sub-group/test.git', "/top-group/sub-group/test")
+        end
+
+        it "handles referencing another top group's project" do
+          expect_relative_link_to_resolve_to('../../frontend/css/test.git', "/frontend/css/test")
+        end
       end
 
       context 'personal project' do
         let(:user) { create(:user) }
         let(:project) { create(:project, namespace: user.namespace) }
 
-        it 'one level down with personal project' do
-          result = relative_self_links('../test.git', commit_id, project)
-          expect(result).to eq(["/#{user.username}/test", "/#{user.username}/test/tree/#{commit_id}"])
+        it 'handles referencing another personal project' do
+          expect_relative_link_to_resolve_to('../test.git', "/#{user.username}/test")
         end
       end
     end