Handle invalid submodule names during recursive updates - #2205
Merged
Byron merged 2 commits intoAug 4, 2026
Merged
Conversation
Copy link
Copy Markdown
Member
Sorry, something went wrong.
<!-- 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>
Copy link
Copy Markdown
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 mrepo before the try in Submodule.update() so keep_going-handled validation failures don’t lead to UnboundLocalError during recursion.
- Extend tests to cover direct recursive=True, keep_going=True updates 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.
Sorry, something went wrong.
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=True and keep_going=True, this validation error is swallowed, but execution then reaches the recursion block and reads mrepo before it has been assigned, raising UnboundLocalError instead of honoring keep_going. Fresh evidence in this revision is that the validation was moved inside the try without initializing mrepo on the non-dry-run path; initialize it before the try or skip recursion after the handled failure.
And in a separate stg commit: _module_abspath() only validates name when _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 validating name unconditionally at the start of the method and then using the validated value in the gitfile-submodules branch.
Changes
Git baseline
Git submodule.c at 883a47ef6496c96a5d6132ed8c87fcd44ebf8d1a validates submodule paths before filesystem operations.
Validation