More hints for invalid submodules

readthedocs/projects/exceptions.py

@@ -40,7 +40,7 @@ class RepositoryError(BuildEnvironmentError):
     )
 
     INVALID_SUBMODULES = _(
-        'One or more submodule URLs are not valid.'
+        'One or more submodule URLs are not valid: {}.'
     )
 
     DUPLICATED_RESERVED_VERSIONS = _(

readthedocs/rtd_tests/tests/test_backend.py

@@ -56,7 +56,6 @@ def test_git_branches(self, checkout_path):
         default_branches = [
             # comes from ``make_test_git`` function
             'submodule',
-            'relativesubmodule',
             'invalidsubmodule',
         ]
         branches = [
@@ -89,7 +88,6 @@ def test_git_branches_unicode(self, checkout_path):
         default_branches = [
             # comes from ``make_test_git`` function
             'submodule',
-            'relativesubmodule',
             'invalidsubmodule',
         ]
         branches = [
@@ -163,15 +161,19 @@ def test_check_submodule_urls(self):
         repo.checkout('submodule')
         valid, _ = repo.validate_submodules(self.dummy_conf)
         self.assertTrue(valid)
-        repo.checkout('relativesubmodule')
-        valid, _ = repo.validate_submodules(self.dummy_conf)
-        self.assertTrue(valid)
 
-    @pytest.mark.xfail(strict=True, reason="Fixture is not working correctly")
     def test_check_invalid_submodule_urls(self):
+        repo = self.project.vcs_repo()
+        repo.update()
+        r = repo.checkout('invalidsubmodule')
         with self.assertRaises(RepositoryError) as e:
-            repo.checkout('invalidsubmodule')
-            self.assertEqual(e.msg, RepositoryError.INVALID_SUBMODULES)
+            repo.update_submodules(self.dummy_conf)
+        # `invalid` is created in `make_test_git`
+        # it's a url in ssh form.
+        self.assertEqual(
+            str(e.exception),
+            RepositoryError.INVALID_SUBMODULES.format(['invalid'])
+        )
 
     @patch('readthedocs.projects.models.Project.checkout_path')
     def test_fetch_clean_tags_and_branches(self, checkout_path):
@@ -197,8 +199,7 @@ def test_fetch_clean_tags_and_branches(self, checkout_path):
         )
         self.assertEqual(
             set([
-                'relativesubmodule', 'invalidsubmodule',
-                'master', 'submodule', 'newbranch',
+                'invalidsubmodule', 'master', 'submodule', 'newbranch',
             ]),
             set(vcs.verbose_name for vcs in repo.branches)
         )
@@ -212,8 +213,7 @@ def test_fetch_clean_tags_and_branches(self, checkout_path):
         )
         self.assertEqual(
             set([
-                'relativesubmodule', 'invalidsubmodule',
-                'master', 'submodule'
+                'invalidsubmodule', 'master', 'submodule'
             ]),
             set(vcs.verbose_name for vcs in repo.branches)
         )

readthedocs/rtd_tests/utils.py

@@ -2,10 +2,15 @@
 """Utility functions for use in tests."""
 
 from __future__ import (
-    absolute_import, division, print_function, unicode_literals)
+    absolute_import,
+    division,
+    print_function,
+    unicode_literals,
+)
 
 import logging
 import subprocess
+import textwrap
 from os import chdir, environ, mkdir
 from os.path import abspath
 from os.path import join as pjoin
@@ -55,34 +60,16 @@ def make_test_git():
     # URL are not allowed and using a real URL will require Internet to clone
     # the repo
     check_output(['git', 'checkout', '-b', 'submodule', 'master'], env=env)
-    # https://stackoverflow.com/a/37378302/2187091
-    mkdir(pjoin(directory, 'foobar'))
-    gitmodules_path = pjoin(directory, '.gitmodules')
-    with open(gitmodules_path, 'w') as fh:
-        fh.write('''[submodule "foobar"]\n\tpath = foobar\n\turl = https://foobar.com/git\n''')
-    check_output(
-        [
-            'git', 'update-index', '--add', '--cacheinfo', '160000',
-            '233febf4846d7a0aeb95b6c28962e06e21d13688', 'foobar',
-        ],
-        env=env,
+    add_git_submodule_without_cloning(
+        directory, 'foobar', 'https://foobar.com/git'
     )
     check_output(['git', 'add', '.'], env=env)
     check_output(['git', 'commit', '-m"Add submodule"'], env=env)
 
-    # Add a relative submodule URL in the relativesubmodule branch
-    check_output(['git', 'checkout', '-b', 'relativesubmodule', 'master'], env=env)
-    check_output(
-        ['git', 'submodule', 'add', '-b', 'master', './', 'relativesubmodule'],
-        env=env
-    )
-    check_output(['git', 'add', '.'], env=env)
-    check_output(['git', 'commit', '-m"Add relative submodule"'], env=env)
     # Add an invalid submodule URL in the invalidsubmodule branch
     check_output(['git', 'checkout', '-b', 'invalidsubmodule', 'master'], env=env)
-    check_output(
-        ['git', 'submodule', 'add', '-b', 'master', './', 'invalidsubmodule'],
-        env=env,
+    add_git_submodule_without_cloning(
+        directory, 'invalid', '[email protected]:rtfd/readthedocs.org.git'
     )
     check_output(['git', 'add', '.'], env=env)
     check_output(['git', 'commit', '-m"Add invalid submodule"'], env=env)
@@ -92,6 +79,43 @@ def make_test_git():
     return directory
 
 
+@restoring_chdir
+def add_git_submodule_without_cloning(directory, submodule, url):
+    """
+    Add a submodule without cloning it.
+
+    We write directly to the git index, more details in:
+    https://stackoverflow.com/a/37378302/2187091
+
+    :param directory: The directory where the git repo is
+    :type directory: str
+    :param submodule: The name of the submodule to be created
+    :type submodule: str
+    :param url: The url where the submodule points to
+    :type url: str
+    """
+    env = environ.copy()
+    env['GIT_DIR'] = pjoin(directory, '.git')
+    chdir(directory)
+
+    mkdir(pjoin(directory, submodule))
+    gitmodules_path = pjoin(directory, '.gitmodules')
+    with open(gitmodules_path, 'w+') as fh:
+        content = textwrap.dedent('''
+            [submodule "{submodule}"]
+                path = {submodule}
+                url = {url}
+        ''')
+        fh.write(content.format(submodule=submodule, url=url))
+    check_output(
+        [
+            'git', 'update-index', '--add', '--cacheinfo', '160000',
+            '233febf4846d7a0aeb95b6c28962e06e21d13688', submodule,
+        ],
+        env=env,
+    )
+
+
 @restoring_chdir
 def make_git_repo(directory, name='sample_repo'):
     path = get_readthedocs_app_path()

readthedocs/vcs_support/backends/git.py

@@ -96,13 +96,16 @@ def validate_submodules(self, config):
 
         :returns: tuple(bool, list)
 
-        Returns true if all required submodules URLs are valid.
+        Returns `True` if all required submodules URLs are valid.
         Returns a list of all required submodules:
         - Include is `ALL`, returns all submodules avaliable.
         - Include is a list, returns just those.
         - Exclude is `ALL` - this should never happen.
         - Exlude is a list, returns all avaliable submodules
           but those from the list.
+
+        Returns `False` if at least one submodule is invalid.
+        Returns the list of invalid submodules.
         """
         repo = git.Repo(self.working_dir)
         submodules = {
@@ -122,11 +125,15 @@ def validate_submodules(self, config):
                 submodules_include[path] = submodules[path]
             submodules = submodules_include
 
+        invalid_submodules = []
         for path, submodule in submodules.items():
             try:
                 validate_submodule_url(submodule.url)
             except ValidationError:
-                return False, []
+                invalid_submodules.append(path)
+
+        if invalid_submodules:
+            return False, invalid_submodules
         return True, submodules.keys()
 
     def fetch(self):
@@ -222,7 +229,9 @@ def update_submodules(self, config):
             if valid:
                 self.checkout_submodules(submodules, config)
             else:
-                raise RepositoryError(RepositoryError.INVALID_SUBMODULES)
+                raise RepositoryError(
+                    RepositoryError.INVALID_SUBMODULES.format(submodules)
+                )
 
     def checkout_submodules(self, submodules, config):
         """Checkout all repository submodules."""