Skip to content

Return None from submodules.get() for a nested plain repository - #1487

Merged
jdavid merged 1 commit into
libgit2:masterfrom
rawsun007:fix-submodules-get-contains
Sep 5, 2026
Merged

jdavid merged 1 commit into
libgit2:masterfrom
rawsun007:fix-submodules-get-contains

Conversation

@rawsun007

Copy link
Copy Markdown
Contributor

Fixes #1405

The bug, and one symptom the issue does not mention

SubmoduleCollection.get() promises:

Unlike __getitem__, this returns None if the submodule is not found.

It only catches KeyError. But git_submodule_lookup reports GIT_EEXISTS, not GIT_ENOTFOUND, when a repository exists at the path yet was never registered as a submodule — libgit2's message is submodule 'x' has not been added yet. check_error turns that into AlreadyExistsError, which escapes get().

Because __contains__ is implemented as self.get(name) is not None, the same input makes a containment test raise:

outer = pygit2.init_repository(outer)
pygit2.init_repository(outer/nested)

outer.submodules.get(nested)      # ValueError: submodule nested has not been added yet
nested in outer.submodules        # same ValueError
outer.submodules.get(absent)      # None  (correct)

Reproduced on 1.20.0 from PyPI and on this branch's parent built against libgit2 1.9.7.

The reporter later wondered whether their confusion about nested repositories was the real issue. It was not the whole of it: whatever one thinks libgit2 should report for a nested repository, get() and in are documented to answer "absent" rather than raise, and today they raise.

The change

get() also catches AlreadyExistsError. __getitem__ is untouched, so callers who need to distinguish "a repository is there but unregistered" from "nothing is there" still can — the new test pins that too.

I considered instead translating GIT_EEXISTS to KeyError inside __getitem__, which would arguably make its own docstring more accurate. I did not, because it changes an exception type users may already catch, and it is your call rather than mine. Happy to switch if you prefer it.

Verification

  • pytest test/test_submodule.py28 passed
  • Full suite minus network/ssh — 595 passed, 7 skipped, 2 xfailed, 1 xpassed, 1 failed
  • That one failure is pre-existing: test_status_file_unicode_normalization[café.txt] fails identically with my changes stashed on a clean checkout, which is the usual macOS filesystem normalisation issue
  • Mutation check: reverting only pygit2/submodules.py fails exactly the new test, test_lookup_nested_repo_that_is_not_a_submodule
  • ruff check and ruff format --check clean on both files
  • Built locally with LIBGIT2=$(brew --prefix libgit2) pip install -e .

Per CONTRIBUTING.md, the commit carries Assisted-by: Claude Code (Claude Opus 5). I can explain the change and stand behind it.

libgit2's git_submodule_lookup reports GIT_EEXISTS, not GIT_ENOTFOUND, when
a repository exists at the path but was never registered as a submodule.
check_error turns that into AlreadyExistsError, which get() did not catch, so
it raised instead of returning None as its docstring promises. __contains__
is built on get(), so `name in repo.submodules` raised for the same input.

__getitem__ keeps raising AlreadyExistsError, so callers that need to tell
"a repository is there" from "nothing is there" still can.

Fixes libgit2#1405
Assisted-by: Claude Code (Claude Opus 5)
@jdavid
jdavid merged commit 87c2a39 into libgit2:master Sep 5, 2026
16 of 18 checks passed
rawsun007 added a commit to rawsun007/pygit2 that referenced this pull request Sep 18, 2026
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 libgit2#1487.

Closes libgit2#1489
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

submodules.get() throws a ValueError exception rather than returning None as documented.

2 participants