Fix the errors related to installed module having invalid commit data (#4086)

redbot/cogs/downloader/downloader.py

@@ -307,9 +307,21 @@ class Downloader(commands.Cog):
         hashes: Dict[Tuple[Repo, str], Set[InstalledModule]] = defaultdict(set)
         for module in modules:
             module.repo = cast(Repo, module.repo)
-            if module.repo.commit != module.commit and await module.repo.is_ancestor(
+            if module.repo.commit != module.commit:
-                module.commit, module.repo.commit
+                try:
-            ):
+                    should_add = await module.repo.is_ancestor(module.commit, module.repo.commit)
+                except errors.UnknownRevision:
+                    # marking module for update if the saved commit data is invalid
+                    last_module_occurrence = await module.repo.get_last_module_occurrence(
+                        module.name
+                    )
+                    if last_module_occurrence is not None and not last_module_occurrence.disabled:
+                        if last_module_occurrence.type == InstallableType.COG:
+                            cogs_to_update.add(last_module_occurrence)
+                        elif last_module_occurrence.type == InstallableType.SHARED_LIBRARY:
+                            libraries_to_update.add(last_module_occurrence)
+                else:
+                    if should_add:
                         hashes[(module.repo, module.commit)].add(module)
 
         update_commits = []

redbot/cogs/downloader/repo_manager.py

@@ -199,6 +199,11 @@ class Repo(RepoJSONMixin):
         descendant_rev : `str`
             Descendant revision
 
+        Raises
+        ------
+        .UnknownRevision
+            When git cannot find one of the provided revisions.
+
         Returns
         -------
         bool
@@ -213,10 +218,17 @@ class Repo(RepoJSONMixin):
             maybe_ancestor_rev=maybe_ancestor_rev,
             descendant_rev=descendant_rev,
         )
-        p = await self._run(git_command, valid_exit_codes=valid_exit_codes)
+        p = await self._run(git_command, valid_exit_codes=valid_exit_codes, debug_only=True)
 
         if p.returncode in valid_exit_codes:
             return not bool(p.returncode)
+
+        # this is a plumbing command so we're safe here
+        stderr = p.stderr.decode(**DECODE_PARAMS).strip()
+        if stderr.startswith(("fatal: Not a valid object name", "fatal: Not a valid commit name")):
+            rev, *__ = stderr[31:].split(maxsplit=1)
+            raise errors.UnknownRevision(f"Revision {rev} cannot be found.", git_command)
+
         raise errors.GitException(
             f"Git failed to determine if commit {maybe_ancestor_rev}"
             f" is ancestor of {descendant_rev} for repo at path: {self.folder_path}",

tests/cogs/downloader/test_downloader.py

@@ -81,14 +81,15 @@ async def test_is_ancestor(mocker, repo, maybe_ancestor_rev, descendant_rev, ret
             descendant_rev=descendant_rev,
         ),
         valid_exit_codes=(0, 1),
+        debug_only=True,
     )
     assert ret is expected
 
 
 @pytest.mark.asyncio
-async def test_is_ancestor_raise(mocker, repo):
+async def test_is_ancestor_object_raise(mocker, repo):
-    m = _mock_run(mocker, repo, 128)
+    m = _mock_run(mocker, repo, 128, b"", b"fatal: Not a valid object name invalid1")
-    with pytest.raises(GitException):
+    with pytest.raises(UnknownRevision):
         await repo.is_ancestor("invalid1", "invalid2")
 
     m.assert_called_once_with(
@@ -99,6 +100,33 @@ async def test_is_ancestor_raise(mocker, repo):
             descendant_rev="invalid2",
         ),
         valid_exit_codes=(0, 1),
+        debug_only=True,
+    )
+
+
+@pytest.mark.asyncio
+async def test_is_ancestor_commit_raise(mocker, repo):
+    m = _mock_run(
+        mocker,
+        repo,
+        128,
+        b"",
+        b"fatal: Not a valid commit name 0123456789abcde0123456789abcde0123456789",
+    )
+    with pytest.raises(UnknownRevision):
+        await repo.is_ancestor(
+            "0123456789abcde0123456789abcde0123456789", "c950fc05a540dd76b944719c2a3302da2e2f3090"
+        )
+
+    m.assert_called_once_with(
+        ProcessFormatter().format(
+            repo.GIT_IS_ANCESTOR,
+            path=repo.folder_path,
+            maybe_ancestor_rev="0123456789abcde0123456789abcde0123456789",
+            descendant_rev="c950fc05a540dd76b944719c2a3302da2e2f3090",
+        ),
+        valid_exit_codes=(0, 1),
+        debug_only=True,
     )
 
 

tests/cogs/downloader/test_git.py

@@ -381,7 +381,7 @@ async def test_git_is_ancestor_false(git_repo):
 
 
 @pytest.mark.asyncio
-async def test_git_is_ancestor_invalid_ref(git_repo):
+async def test_git_is_ancestor_invalid_object(git_repo):
     p = await git_repo._run(
         ProcessFormatter().format(
             git_repo.GIT_IS_ANCESTOR,
@@ -394,6 +394,22 @@ async def test_git_is_ancestor_invalid_ref(git_repo):
     assert p.stderr.decode().strip() == "fatal: Not a valid object name invalid1"
 
 
+@pytest.mark.asyncio
+async def test_git_is_ancestor_invalid_commit(git_repo):
+    p = await git_repo._run(
+        ProcessFormatter().format(
+            git_repo.GIT_IS_ANCESTOR,
+            path=git_repo.folder_path,
+            maybe_ancestor_rev="0123456789abcde0123456789abcde0123456789",
+            descendant_rev="c950fc05a540dd76b944719c2a3302da2e2f3090",
+        )
+    )
+    assert p.returncode == 128
+    assert p.stderr.decode().strip() == (
+        "fatal: Not a valid commit name 0123456789abcde0123456789abcde0123456789"
+    )
+
+
 @pytest.mark.asyncio
 async def test_git_check_if_module_exists_true(git_repo):
     p = await git_repo._run(