Handle invalid submodule names during recursive updates - #2205
Merged
Conversation
<!-- agent --> Direct recursive updates with keep_going could swallow an invalid-name validation error and then access mrepo before assignment. Reproduce that path and initialize mrepo before validation so recursion is safely skipped. Git baseline: submodule.c at 883a47ef6496c96a5d6132ed8c87fcd44ebf8d1a validates submodule paths before operating on them. Assisted-by: GPT 5.6 Co-authored-by: GPT 5.6 <codex@openai.com>
<!-- agent --> _module_abspath only validated names when Git used separate gitfile submodule directories, so legacy Git behavior could accept the same invalid name. Validate before selecting the Git-version-dependent path branch and cover the legacy branch explicitly. Git baseline: submodule.c at 883a47ef6496c96a5d6132ed8c87fcd44ebf8d1a validates submodule paths before filesystem operations. Assisted-by: GPT 5.6 Co-authored-by: GPT 5.6 <codex@openai.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Fixes a crash path in recursive submodule updates when invalid submodule names are encountered with keep_going=True, and makes submodule-name validation consistent across Git-version-dependent code paths.
Changes:
- Validate submodule names unconditionally in
Submodule._module_abspath()so invalid names fail consistently regardless of_need_gitfile_submodules(). - Initialize
mrepobefore thetryinSubmodule.update()sokeep_going-handled validation failures don’t lead toUnboundLocalErrorduring recursion. - Extend tests to cover direct
recursive=True, keep_going=Trueupdates on invalid submodules and the legacy module-path branch.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
test/test_submodule.py |
Adds regression coverage for invalid-name handling across both module-path branches and direct recursive updates with keep_going=True. |
git/objects/submodule/base.py |
Ensures name validation is applied consistently and prevents mrepo from being used before assignment when errors are swallowed. |
.basedpyright/baseline.json |
Removes a now-fixed “possibly unbound variable” baseline entry consistent with the mrepo initialization change. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Merged
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.
Tasks
This section is for Byron only. Models continuing this PR must not add, remove, check, uncheck, rename, or reorder checkboxes here.
Everything below this line was generated by Codex GPT-5.
Created by Codex on behalf of Byron. Byron will review before this is ready to merge.
Reported issue
When an invalidly named submodule is updated directly with both
recursive=Trueandkeep_going=True, this validation error is swallowed, but execution then reaches the recursion block and readsmrepobefore it has been assigned, raisingUnboundLocalErrorinstead of honoringkeep_going. Fresh evidence in this revision is that the validation was moved inside thetrywithout initializingmrepoon the non-dry-run path; initialize it before thetryor skip recursion after the handled failure.And in a separate stg commit: _module_abspath() only validates
namewhen_need_gitfile_submodules()is true. This makes behavior depend on the installed git version and can allow invalid names to pass through this helper silently on older git versions (even though other code paths now expect invalid names to raise consistently). Consider validatingnameunconditionally at the start of the method and then using the validated value in the gitfile-submodules branch.Changes
keep_goingupdates and the legacy module-path branch.Git baseline
Git
submodule.cat883a47ef6496c96a5d6132ed8c87fcd44ebf8d1avalidates submodule paths before filesystem operations.Validation
test/test_submodule.py: 40 passed, 1 skipped, 1 xfailed.git diff --check.