Return None from submodules.get() for a nested plain repository - #1487
Open
rawsun007 wants to merge 1 commit into
Open
Return None from submodules.get() for a nested plain repository#1487rawsun007 wants to merge 1 commit into
rawsun007 wants to merge 1 commit into
Conversation
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)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #1405
The bug, and one symptom the issue does not mention
SubmoduleCollection.get()promises:It only catches
KeyError. Butgit_submodule_lookupreportsGIT_EEXISTS, notGIT_ENOTFOUND, when a repository exists at the path yet was never registered as a submodule — libgit2's message issubmodule 'x' has not been added yet.check_errorturns that intoAlreadyExistsError, which escapesget().Because
__contains__is implemented asself.get(name) is not None, the same input makes a containment test raise: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()andinare documented to answer "absent" rather than raise, and today they raise.The change
get()also catchesAlreadyExistsError.__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_EEXISTStoKeyErrorinside__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.py— 28 passedtest_status_file_unicode_normalization[café.txt]fails identically with my changes stashed on a clean checkout, which is the usual macOS filesystem normalisation issuepygit2/submodules.pyfails exactly the new test,test_lookup_nested_repo_that_is_not_a_submoduleruff checkandruff format --checkclean on both filesLIBGIT2=$(brew --prefix libgit2) pip install -e .Per
CONTRIBUTING.md, the commit carriesAssisted-by: Claude Code (Claude Opus 5). I can explain the change and stand behind it.