diff --git a/redbot/cogs/downloader/downloader.py b/redbot/cogs/downloader/downloader.py index ef4138387..19f8ddc74 100644 --- a/redbot/cogs/downloader/downloader.py +++ b/redbot/cogs/downloader/downloader.py @@ -303,10 +303,22 @@ 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( - module.commit, module.repo.commit - ): - hashes[(module.repo, module.commit)].add(module) + if module.repo.commit != module.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 = [] for (repo, old_hash), modules_to_check in hashes.items(): diff --git a/redbot/cogs/downloader/repo_manager.py b/redbot/cogs/downloader/repo_manager.py index d288af026..bd8de2bc5 100644 --- a/redbot/cogs/downloader/repo_manager.py +++ b/redbot/cogs/downloader/repo_manager.py @@ -198,6 +198,11 @@ class Repo(RepoJSONMixin): descendant_rev : `str` Descendant revision + Raises + ------ + .UnknownRevision + When git cannot find one of the provided revisions. + Returns ------- bool @@ -212,10 +217,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}", diff --git a/tests/cogs/downloader/test_downloader.py b/tests/cogs/downloader/test_downloader.py index eebff2bb3..71e3b811c 100644 --- a/tests/cogs/downloader/test_downloader.py +++ b/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): - m = _mock_run(mocker, repo, 128) - with pytest.raises(GitException): +async def test_is_ancestor_object_raise(mocker, repo): + m = _mock_run(mocker, repo, 128, b"", b"fatal: Not a valid object name invalid1") + 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, ) diff --git a/tests/cogs/downloader/test_git.py b/tests/cogs/downloader/test_git.py index 075b6e783..f7bc40d5d 100644 --- a/tests/cogs/downloader/test_git.py +++ b/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(