Uh oh!
There was an error while loading. Please reload this page.
fixup 2202 - #2206
Conversation
<!-- 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>
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)whenlist_itemsfails, and forCommit._iter_from_process_or_streaminvalid 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
reportPossiblyUnboundVariablesuppressions.
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. |
Suppressed comments (1)
git/repo/base.py:943
- Avoid using
assertfor runtime validation in library code. Ifgit_diris unexpectedly missing, raise an explicit exception rather than relying on an assertion (which can be optimized away) and risking a laterTypeErrorfromosp.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.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
<!-- 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>
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_exporthas the same empty-stringgit_dirbehavior as_get_daemon_exportand may create/removegit-daemon-export-okin the current working directory ifgit_diris "". 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_exportnow treats an empty stringgit_diras a valid directory and will probe forgit-daemon-export-okrelative to the current working directory. This differs from the previous truthiness check and fromcurrently_rebasing_on()(which treats falsygit_diras absent) and can lead to reading the wrong path whengit_diris "".
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)
Uh oh!
There was an error while loading. Please reload this page.
Fixup #2205, which missed 2 commits when accidentally merged.