fixup 2202 - #2206
Merged
Byron merged 2 commits intoAug 4, 2026
Merged
Conversation
Copy link
Copy Markdown
Member
Sorry, something went wrong.
<!-- agent --> RootModule.update could swallow setup failures with keep_going and then iterate sms before assignment. Start with an empty submodule list so the handled failure safely skips updates. Commit iteration could likewise read stream before assignment for processes without stdout or unsupported inputs. Raise explicit input errors instead. 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
Fixup follow-up to #2205 to address missed commits by hardening a few code paths against “possibly unbound” locals (including keep_going handling in recursive submodule updates) and adding regression tests for the corrected behaviors.
Changes:
- Add regression coverage for RootModule.update(keep_going=True) when list_items fails, and for Commit._iter_from_process_or_stream invalid inputs.
- Initialize sentinel locals (sms, line_str, line, is_detached, rref, etc.) to avoid unbound-variable crashes and satisfy static analysis.
- Update basedpyright baseline to remove now-unnecessary reportPossiblyUnboundVariable suppressions.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 2 comments.
Show a summary per file| File | Description |
|---|---|
| test/test_submodule.py | Adds regression test ensuring keep_going=True swallows a list_items failure without later unbound-local errors. |
| test/test_commit.py | Adds regression test for explicit exceptions when _iter_from_process_or_stream receives invalid sources. |
| git/repo/base.py | Tightens a few flows to avoid unbound locals (blame parsing, rebasing check) and changes daemon export helpers. |
| git/refs/log.py | Initializes line to avoid unbound-local reports in entry_at. |
| git/objects/submodule/root.py | Ensures sms is always initialized so keep_going paths don’t trip unbound locals; imports IterableList at runtime. |
| git/objects/submodule/base.py | Initializes variables (is_detached, rref) to avoid unbound locals in update/remove flows. |
| git/objects/commit.py | Raises explicit ValueError/TypeError instead of hitting unbound-local errors for invalid process/stream inputs. |
| git/index/base.py | Avoids unbound local for key; asserts working tree presence when rewriting paths. |
| .basedpyright/baseline.json | Removes baseline entries for unbound-variable warnings that are now fixed. |
git/repo/base.py:943
- Avoid using assert for runtime validation in library code. If git_dir is unexpectedly missing, raise an explicit exception rather than relying on an assertion (which can be optimized away) and risking a later TypeError from osp.join.
def _set_daemon_export(self, value: object) -> None:
assert self.git_dir is not None, "Daemon export requires a Git directory"
filename = osp.join(self.git_dir, self.DAEMON_EXPORT_FILE)
fileexists = osp.exists(filename)
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Sorry, something went wrong.
<!-- agent --> Make established repository invariants explicit, initialize loop-only locals, and move assignments ahead of exception handling so basedpyright can follow the existing control flow. Regenerate the baseline to remove all 19 reportPossiblyUnboundVariable suppressions, including the two runtime fixes from the preceding commit. Validation: basedpyright --warnings; unbaselined possibly-unbound count 0; eight focused subsystem tests passed. 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
Copilot reviewed 10 out of 10 changed files in this pull request and generated no new comments.
Suppressed comments (2)git/repo/base.py:947
- _set_daemon_export has the same empty-string git_dir behavior as _get_daemon_export and may create/remove git-daemon-export-ok in the current working directory if git_dir is "". Treat falsy values consistently as "no git_dir".
def _set_daemon_export(self, value: object) -> None:
git_dir = getattr(self, "git_dir", None)
if git_dir is None:
return
filename = osp.join(git_dir, self.DAEMON_EXPORT_FILE)
fileexists = osp.exists(filename)
git/repo/base.py:940
- _get_daemon_export now treats an empty string git_dir as a valid directory and will probe for git-daemon-export-ok relative to the current working directory. This differs from the previous truthiness check and from currently_rebasing_on() (which treats falsy git_dir as absent) and can lead to reading the wrong path when git_dir is "".
This issue also appears on line 942 of the same file.
git_dir = getattr(self, "git_dir", None)
if git_dir is None:
return False
filename = osp.join(git_dir, self.DAEMON_EXPORT_FILE)
return osp.exists(filename)
Sorry, something went wrong.
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.
Fixup #2205, which missed 2 commits when accidentally merged.