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

Return None from submodules.get() for a nested plain repository#1487
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
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