From c673bb0979c003d0aa6b179085532d9b284b2376 Mon Sep 17 00:00:00 2001 From: jack1142 <6032823+jack1142@users.noreply.github.com> Date: Wed, 5 Aug 2020 20:35:57 +0200 Subject: [PATCH] Fix the errors related to installed module having invalid commit data (#4086) --- redbot/cogs/downloader/downloader.py | 20 +++++++++++--- redbot/cogs/downloader/repo_manager.py | 14 +++++++++- tests/cogs/downloader/test_downloader.py | 34 +++++++++++++++++++++--- tests/cogs/downloader/test_git.py | 18 ++++++++++++- 4 files changed, 77 insertions(+), 9 deletions(-) diff --git a/redbot/cogs/downloader/downloader.py b/redbot/cogs/downloader/downloader.py index 1c8666650..f4888e441 100644 --- a/redbot/cogs/downloader/downloader.py +++ b/redbot/cogs/downloader/downloader.py @@ -307,10 +307,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 abbe680da..348a50328 100644 --- a/redbot/cogs/downloader/repo_manager.py +++ b/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}", 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(