From 58d81342e91c71ca8aabe4d94eee9aff08d7e4ef Mon Sep 17 00:00:00 2001 From: rawsun007 Date: Fri, 18 Sep 2026 10:47:54 +0530 Subject: [PATCH] References.get and Branches.get return None for an invalid name Both are documented to return None when the lookup fails, and both catch only KeyError. libgit2 reports two codes for a failed lookup: GIT_ENOTFOUND, which errors.py maps to NotFoundError (a KeyError), and GIT_EINVALIDSPEC, which maps to InvalidSpecError (a ValueError). Only the first reaches the handler. repo.references.get('master') InvalidSpecError 'master' in repo.references InvalidSpecError repo.branches.get('my branch') InvalidSpecError 'my branch' in repo.branches InvalidSpecError __contains__ is built on get() in both collections, so it raises too. Branches.get is now typed Branch | None, dropping a "# type:ignore # next commit". That makes mypy see what the signature always meant, so the tests that call it and then use the result assert it is not None. Same shape as the SubmoduleCollection.get fix in #1487. Closes #1489 --- pygit2/branches.py | 10 ++++++---- pygit2/references.py | 6 +++++- test/test_branch.py | 21 +++++++++++++++++++++ test/test_branch_empty.py | 1 + test/test_packbuilder.py | 1 + test/test_refs.py | 20 ++++++++++++++++++++ 6 files changed, 54 insertions(+), 5 deletions(-) diff --git a/pygit2/branches.py b/pygit2/branches.py index 77cabfc1..5819333f 100644 --- a/pygit2/branches.py +++ b/pygit2/branches.py @@ -28,7 +28,7 @@ from collections.abc import Iterator from typing import TYPE_CHECKING -from ._pygit2 import Branch, Commit, Oid +from ._pygit2 import Branch, Commit, InvalidSpecError, Oid from .enums import BranchType, ReferenceType # Need BaseRepository for type hints, but don't let it cause a circular dependency @@ -72,11 +72,13 @@ def __getitem__(self, name: str) -> Branch: return branch - def get(self, key: str) -> Branch: + def get(self, key: str) -> Branch | None: try: return self[key] - except KeyError: - return None # type:ignore # next commit + except (KeyError, InvalidSpecError): + # As in References.get: git_branch_lookup reports + # GIT_EINVALIDSPEC for a name that is not a valid branch name. + return None def __iter__(self) -> Iterator[str]: for branch_name in self._repository.listall_branches(self._flag): diff --git a/pygit2/references.py b/pygit2/references.py index 533e90ab..4094bb56 100644 --- a/pygit2/references.py +++ b/pygit2/references.py @@ -30,6 +30,7 @@ from pygit2 import Oid +from ._pygit2 import InvalidSpecError from .enums import ReferenceFilter # Need BaseRepository for type hints, but don't let it cause a circular dependency @@ -48,7 +49,10 @@ def __getitem__(self, name: str) -> 'Reference': def get(self, key: str) -> 'Reference' | None: try: return self[key] - except KeyError: + except (KeyError, InvalidSpecError): + # git_reference_lookup reports GIT_EINVALIDSPEC as well as + # GIT_ENOTFOUND for a name it cannot resolve; only the latter + # arrives as a KeyError. return None def __iter__(self) -> Iterator[str]: diff --git a/test/test_branch.py b/test/test_branch.py index 14cb965f..e6f5940c 100644 --- a/test/test_branch.py +++ b/test/test_branch.py @@ -85,11 +85,13 @@ def test_branches_delete_error(testrepo: Repository) -> None: def test_branches_is_head(testrepo: Repository) -> None: branch = testrepo.branches.get('master') + assert branch is not None assert branch.is_head() def test_branches_is_not_head(testrepo: Repository) -> None: branch = testrepo.branches.get('i18n') + assert branch is not None assert not branch.is_head() @@ -98,34 +100,40 @@ def test_branches_rename(testrepo: Repository) -> None: assert new_branch.target == I18N_LAST_COMMIT new_branch_2 = testrepo.branches.get('new-branch') + assert new_branch_2 is not None assert new_branch_2.target == I18N_LAST_COMMIT def test_branches_rename_error(testrepo: Repository) -> None: original_branch = testrepo.branches.get('i18n') + assert original_branch is not None with pytest.raises(ValueError): original_branch.rename('master') def test_branches_rename_force(testrepo: Repository) -> None: original_branch = testrepo.branches.get('master') + assert original_branch is not None new_branch = original_branch.rename('i18n', True) assert new_branch.target == LAST_COMMIT def test_branches_rename_invalid(testrepo: Repository) -> None: original_branch = testrepo.branches.get('i18n') + assert original_branch is not None with pytest.raises(ValueError): original_branch.rename('abc@{123') def test_branches_name(testrepo: Repository) -> None: branch = testrepo.branches.get('master') + assert branch is not None assert branch.branch_name == 'master' assert branch.name == 'refs/heads/master' assert branch.raw_branch_name == branch.branch_name.encode('utf-8') branch = testrepo.branches.get('i18n') + assert branch is not None assert branch.branch_name == 'i18n' assert branch.name == 'refs/heads/i18n' assert branch.raw_branch_name == branch.branch_name.encode('utf-8') @@ -268,3 +276,16 @@ def test_branch_name(testrepo: Repository) -> None: branch = testrepo.lookup_branch('i18n') assert branch.branch_name == 'i18n' assert branch.name == 'refs/heads/i18n' + + +def test_branches_get_invalid_name(testrepo: Repository) -> None: + """Branches.get() returns None for a name that is not a valid branch name. + + git_branch_lookup reports GIT_EINVALIDSPEC for these, which arrives as + InvalidSpecError rather than KeyError. + """ + repo = testrepo + + assert repo.branches.get('my branch') is None + assert 'my branch' not in repo.branches + assert repo.branches.get('does-not-exist') is None diff --git a/test/test_branch_empty.py b/test/test_branch_empty.py index b0ef793c..7934c60b 100644 --- a/test/test_branch_empty.py +++ b/test/test_branch_empty.py @@ -42,6 +42,7 @@ def repo(emptyrepo: Repository) -> Generator[Repository, None, None]: def test_branches_remote_get(repo: Repository) -> None: branch = repo.branches.remote.get('origin/master') + assert branch is not None assert branch.target == ORIGIN_MASTER_COMMIT assert repo.branches.remote.get('origin/not-exists') is None diff --git a/test/test_packbuilder.py b/test/test_packbuilder.py index 06410699..c3552c0c 100644 --- a/test/test_packbuilder.py +++ b/test/test_packbuilder.py @@ -75,6 +75,7 @@ def test_pack_with_delegate(testrepo: Repository, tmp_path: Path) -> None: def pack_delegate(pb: PackBuilder) -> None: for branch in pb._repo.branches: br = pb._repo.branches.get(branch) + assert br is not None for commit in br.log(): pb.add_recur(commit.oid_new) diff --git a/test/test_refs.py b/test/test_refs.py index 80c59fc9..f2e35fdb 100644 --- a/test/test_refs.py +++ b/test/test_refs.py @@ -466,6 +466,26 @@ def test_lookup_reference(testrepo: Repository) -> None: assert reference.name == 'refs/heads/master' +def test_references_get_invalid_name(testrepo: Repository) -> None: + """A name libgit2 rejects as a spec is still "not found" to get(). + + git_reference_lookup reports GIT_EINVALIDSPEC as well as GIT_ENOTFOUND, + and only the latter reaches pygit2 as a KeyError. + """ + repo = testrepo + + # 'master' is not a full reference name, so libgit2 calls it invalid. + assert repo.references.get('master') is None + assert 'master' not in repo.references + + # A well-formed name that does not exist behaves the same way. + assert repo.references.get('refs/heads/does-not-exist') is None + + # __getitem__ still raises, as documented. + with pytest.raises((KeyError, InvalidSpecError)): + repo.references['master'] + + def test_lookup_reference_dwim(testrepo: Repository) -> None: repo = testrepo