[Downloader] More robust repo loading (#2121)

Previously, when downloader was loaded, the RepoManager would spawn a task to load available repos. If one repo failed loading for some reason, the function would raise and the remaining repos would never be loaded, however downloader would still appear to load correctly.

This change handles exceptions better during repo loading, but also, if an unhandled exception is raised, downloader will fail to load as it should.

Also included, as requested in #1968, is the --recurse-submodules flag in cloning/pulling repositories.

This change resolves #1950.

Signed-off-by: Toby Harradine <tobyharradine@gmail.com>

redbot/cogs/downloader/__init__.py

@@ -1,6 +1,7 @@
-from redbot.core.bot import Red
 from .downloader import Downloader
 
 
-def setup(bot: Red):
+async def setup(bot):
-    bot.add_cog(Downloader(bot))
+    cog = Downloader(bot)
+    await cog.initialize()
+    bot.add_cog(cog)

redbot/cogs/downloader/downloader.py

@@ -1,23 +1,20 @@
 import os
 import shutil
+import sys
 from pathlib import Path
 from sys import path as syspath
 from typing import Tuple, Union
 
 import discord
-import sys
+from redbot.core import checks, commands, Config
-
+from redbot.core.bot import Red
-from redbot.core import Config
-from redbot.core import checks
 from redbot.core.data_manager import cog_data_path
 from redbot.core.i18n import Translator, cog_i18n
 from redbot.core.utils.chat_formatting import box, pagify
-from redbot.core import commands
 
-from redbot.core.bot import Red
+from . import errors
 from .checks import do_install_agreement
 from .converters import InstalledCog
-from .errors import CloningError, ExistingGitRepo
 from .installable import Installable
 from .log import log
 from .repo_manager import RepoManager, Repo
@@ -51,6 +48,9 @@ class Downloader:
 
         self._repo_manager = RepoManager()
 
+    async def initialize(self):
+        await self._repo_manager.initialize()
+
     async def cog_install_path(self):
         """Get the current cog install path.
 
@@ -226,11 +226,16 @@ class Downloader:
         try:
             # noinspection PyTypeChecker
             repo = await self._repo_manager.add_repo(name=name, url=repo_url, branch=branch)
-        except ExistingGitRepo:
+        except errors.ExistingGitRepo:
             await ctx.send(_("That git repo has already been added under another name."))
-        except CloningError:
+        except errors.CloningError as err:
             await ctx.send(_("Something went wrong during the cloning process."))
-            log.exception(_("Something went wrong during the cloning process."))
+            log.exception(
+                "Something went wrong whilst cloning %s (to revision: %s)",
+                repo_url,
+                branch,
+                exc_info=err,
+            )
         else:
             await ctx.send(_("Repo `{}` successfully added.").format(name))
             if repo.install_msg is not None:

redbot/cogs/downloader/errors.py

@@ -9,6 +9,7 @@ __all__ = [
     "HardResetError",
     "UpdateError",
     "GitDiffError",
+    "NoRemoteURL",
     "PipError",
 ]
 
@@ -96,6 +97,14 @@ class GitDiffError(GitException):
     pass
 
 
+class NoRemoteURL(GitException):
+    """
+    Thrown when no remote URL exists for a repo.
+    """
+
+    pass
+
+
 class PipError(DownloaderException):
     """
     Thrown when pip returns a non-zero return code.

redbot/cogs/downloader/repo_manager.py

@@ -2,6 +2,7 @@ import asyncio
 import functools
 import os
 import pkgutil
+import shutil
 import re
 from concurrent.futures import ThreadPoolExecutor
 from pathlib import Path
@@ -11,19 +12,19 @@ from typing import Tuple, MutableMapping, Union, Optional
 
 from redbot.core import data_manager, commands
 from redbot.core.utils import safe_delete
-from .errors import *
+from . import errors
 from .installable import Installable, InstallableType
 from .json_mixins import RepoJSONMixin
 from .log import log
 
 
 class Repo(RepoJSONMixin):
-    GIT_CLONE = "git clone -b {branch} {url} {folder}"
+    GIT_CLONE = "git clone --recurse-submodules -b {branch} {url} {folder}"
-    GIT_CLONE_NO_BRANCH = "git clone {url} {folder}"
+    GIT_CLONE_NO_BRANCH = "git clone --recurse-submodules {url} {folder}"
     GIT_CURRENT_BRANCH = "git -C {path} rev-parse --abbrev-ref HEAD"
     GIT_LATEST_COMMIT = "git -C {path} rev-parse {branch}"
     GIT_HARD_RESET = "git -C {path} reset --hard origin/{branch} -q"
-    GIT_PULL = "git -C {path} pull -q --ff-only"
+    GIT_PULL = "git -C {path}  --recurse-submodules=yes -q --ff-only"
     GIT_DIFF_FILE_STATUS = "git -C {path} diff --no-commit-id --name-status {old_hash} {new_hash}"
     GIT_LOG = "git -C {path} log --relative-date --reverse {old_hash}.. {relative_file_path}"
     GIT_DISCOVER_REMOTE_URL = "git -C {path} config --get remote.origin.url"
@@ -93,7 +94,9 @@ class Repo(RepoJSONMixin):
         )
 
         if p.returncode != 0:
-            raise GitDiffError("Git diff failed for repo at path: {}".format(self.folder_path))
+            raise errors.GitDiffError(
+                "Git diff failed for repo at path: {}".format(self.folder_path)
+            )
 
         stdout = p.stdout.strip().decode().split("\n")
 
@@ -123,7 +126,7 @@ class Repo(RepoJSONMixin):
         )
 
         if p.returncode != 0:
-            raise GitException(
+            raise errors.GitException(
                 "An exception occurred while executing git log on"
                 " this repo: {}".format(self.folder_path)
             )
@@ -177,7 +180,7 @@ class Repo(RepoJSONMixin):
         """
         exists, path = self._existing_git_repo()
         if exists:
-            raise ExistingGitRepo("A git repo already exists at path: {}".format(path))
+            raise errors.ExistingGitRepo("A git repo already exists at path: {}".format(path))
 
         if self.branch is not None:
             p = await self._run(
@@ -190,8 +193,10 @@ class Repo(RepoJSONMixin):
                 self.GIT_CLONE_NO_BRANCH.format(url=self.url, folder=self.folder_path).split()
             )
 
-        if p.returncode != 0:
+        if p.returncode:
-            raise CloningError("Error when running git clone.")
+            # Try cleaning up folder
+            shutil.rmtree(str(self.folder_path), ignore_errors=True)
+            raise errors.CloningError("Error when running git clone.")
 
         if self.branch is None:
             self.branch = await self.current_branch()
@@ -211,12 +216,14 @@ class Repo(RepoJSONMixin):
         """
         exists, _ = self._existing_git_repo()
         if not exists:
-            raise MissingGitRepo("A git repo does not exist at path: {}".format(self.folder_path))
+            raise errors.MissingGitRepo(
+                "A git repo does not exist at path: {}".format(self.folder_path)
+            )
 
         p = await self._run(self.GIT_CURRENT_BRANCH.format(path=self.folder_path).split())
 
         if p.returncode != 0:
-            raise GitException(
+            raise errors.GitException(
                 "Could not determine current branch at path: {}".format(self.folder_path)
             )
 
@@ -241,14 +248,16 @@ class Repo(RepoJSONMixin):
 
         exists, _ = self._existing_git_repo()
         if not exists:
-            raise MissingGitRepo("A git repo does not exist at path: {}".format(self.folder_path))
+            raise errors.MissingGitRepo(
+                "A git repo does not exist at path: {}".format(self.folder_path)
+            )
 
         p = await self._run(
             self.GIT_LATEST_COMMIT.format(path=self.folder_path, branch=branch).split()
         )
 
         if p.returncode != 0:
-            raise CurrentHashError("Unable to determine old commit hash.")
+            raise errors.CurrentHashError("Unable to determine old commit hash.")
 
         return p.stdout.decode().strip()
 
@@ -268,8 +277,9 @@ class Repo(RepoJSONMixin):
 
         Raises
         ------
-        RuntimeError
+        .NoRemoteURL
             When the folder does not contain a git repo with a FETCH URL.
+
         """
         if folder is None:
             folder = self.folder_path
@@ -277,7 +287,7 @@ class Repo(RepoJSONMixin):
         p = await self._run(Repo.GIT_DISCOVER_REMOTE_URL.format(path=folder).split())
 
         if p.returncode != 0:
-            raise RuntimeError("Unable to discover a repo URL.")
+            raise errors.NoRemoteURL("Unable to discover a repo URL.")
 
         return p.stdout.decode().strip()
 
@@ -295,14 +305,16 @@ class Repo(RepoJSONMixin):
 
         exists, _ = self._existing_git_repo()
         if not exists:
-            raise MissingGitRepo("A git repo does not exist at path: {}".format(self.folder_path))
+            raise errors.MissingGitRepo(
+                "A git repo does not exist at path: {}".format(self.folder_path)
+            )
 
         p = await self._run(
             self.GIT_HARD_RESET.format(path=self.folder_path, branch=branch).split()
         )
 
         if p.returncode != 0:
-            raise HardResetError(
+            raise errors.HardResetError(
                 "Some error occurred when trying to"
                 " execute a hard reset on the repo at"
                 " the following path: {}".format(self.folder_path)
@@ -325,7 +337,7 @@ class Repo(RepoJSONMixin):
         p = await self._run(self.GIT_PULL.format(path=self.folder_path).split())
 
         if p.returncode != 0:
-            raise UpdateError(
+            raise errors.UpdateError(
                 "Git pull returned a non zero exit code"
                 " for the repo located at path: {}".format(self.folder_path)
             )
@@ -354,7 +366,7 @@ class Repo(RepoJSONMixin):
 
         """
         if cog not in self.available_cogs:
-            raise DownloaderException("That cog does not exist in this repo")
+            raise errors.DownloaderException("That cog does not exist in this repo")
 
         if not target_dir.is_dir():
             raise ValueError("That target directory is not actually a directory.")
@@ -500,6 +512,9 @@ class RepoManager:
         loop = asyncio.get_event_loop()
         loop.create_task(self._load_repos(set=True))  # str_name: Repo
 
+    async def initialize(self):
+        await self._load_repos(set=True)
+
     @property
     def repos_folder(self) -> Path:
         data_folder = data_manager.cog_data_path(self)
@@ -511,7 +526,7 @@ class RepoManager:
     @staticmethod
     def validate_and_normalize_repo_name(name: str) -> str:
         if not name.isidentifier():
-            raise InvalidRepoName("Not a valid Python variable name.")
+            raise errors.InvalidRepoName("Not a valid Python variable name.")
         return name.lower()
 
     async def add_repo(self, url: str, name: str, branch: Optional[str] = None) -> Repo:
@@ -533,7 +548,7 @@ class RepoManager:
 
         """
         if self.does_repo_exist(name):
-            raise ExistingGitRepo(
+            raise errors.ExistingGitRepo(
                 "That repo name you provided already exists. Please choose another."
             )
 
@@ -584,13 +599,13 @@ class RepoManager:
 
         Raises
         ------
-        MissingGitRepo
+        .MissingGitRepo
             If the repo does not exist.
 
         """
         repo = self.get_repo(name)
         if repo is None:
-            raise MissingGitRepo("There is no repo with the name {}".format(name))
+            raise errors.MissingGitRepo("There is no repo with the name {}".format(name))
 
         safe_delete(repo.folder_path)
 
@@ -629,9 +644,16 @@ class RepoManager:
                 continue
             try:
                 ret[folder.stem] = await Repo.from_folder(folder)
-            except RuntimeError:
+            except errors.NoRemoteURL:
-                # Thrown when there's no findable git remote URL
+                log.warning("A remote URL does not exist for repo %s", folder.stem)
-                pass
+            except errors.DownloaderException as err:
+                log.error("Discarding repo %s due to error.", folder.stem, exc_info=err)
+                shutil.rmtree(
+                    str(folder),
+                    onerror=lambda func, path, exc: log.error(
+                        "Failed to remove folder %s", path, exc_info=exc
+                    ),
+                )
 
         if set:
             self._repos = ret

tests/cogs/downloader/test_downloader.py

@@ -28,20 +28,6 @@ def test_existing_git_repo(tmpdir):
     assert exists is True
 
 
-@pytest.mark.asyncio
-async def test_clone_repo(repo_norun, capsys):
-    await repo_norun.clone()
-
-    clone_cmd, _ = capsys.readouterr()
-    clone_cmd = clone_cmd.strip("[']\n").split("', '")
-    assert clone_cmd[0] == "git"
-    assert clone_cmd[1] == "clone"
-    assert clone_cmd[2] == "-b"
-    assert clone_cmd[3] == "rewrite_cogs"
-    assert clone_cmd[4] == repo_norun.url
-    assert ("repos", "squid") == pathlib.Path(clone_cmd[5]).parts[-2:]
-
-
 @pytest.mark.asyncio
 async def test_add_repo(monkeypatch, repo_manager):
     monkeypatch.setattr("redbot.cogs.downloader.repo_manager.Repo._run", fake_run_noprint)