Broad exception swallowing in find_git_ref hides repository/backend failures#3112
Conversation
…ry/backend failures Signed-off-by: Nguyen Huy Hoang <181364121+huyhoang171106@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR fixes a silent-failure bug in Git reference lookup by removing broad exception swallowing in find_git_ref, so repository/backend failures are no longer indistinguishable from a normal “ref not found” result.
Changes:
- Removed
try/except Exception: passaroundrepo.list_references()and ref matching logic infind_git_ref. - Allows backend/repository exceptions to propagate to callers instead of returning
Nonesilently.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| references = repo.list_references() | ||
|
|
There was a problem hiding this comment.
find_git_ref no longer swallows exceptions from repo.list_references(), but the docstring still implies this function only returns GitRef/None. Please update the docstring (and any callers’ expectations) to note that backend/repository errors can propagate so failures aren’t mistaken for “ref not found”.
| references = repo.list_references() | ||
|
|
||
| # Check shortname first (most common case) | ||
| for git_ref in references: | ||
| if git_ref.shortname == ref_name: | ||
| return git_ref | ||
| # Check shortname first (most common case) | ||
| for git_ref in references: | ||
| if git_ref.shortname == ref_name: | ||
| return git_ref | ||
|
|
||
| # Check full name | ||
| for git_ref in references: | ||
| if git_ref.name == ref_name: | ||
| return git_ref | ||
|
|
||
| except Exception: | ||
| pass | ||
| # Check full name | ||
| for git_ref in references: | ||
| if git_ref.name == ref_name: |
There was a problem hiding this comment.
references is iterated twice (shortname pass, then full-name pass). This is unnecessary work and will behave incorrectly if repo.list_references() returns an iterator/generator (the second loop will be empty). Consider materializing references once (e.g., list(...)) or doing both checks in a single pass.
| references = repo.list_references() | ||
|
|
There was a problem hiding this comment.
This change intentionally alters behavior from “return None on any exception” to “propagate errors”. Please add a unit test covering the new behavior (e.g., when list_references() raises) so regressions don’t reintroduce exception swallowing or silent failures.
phw
left a comment
phw
left a comment
There was a problem hiding this comment.
Thanks for opening a PR, but this change is incomplete. For one tests are failing. But also this PR changes behavior at quite some critical level, without actually handling the impact of the change higher up in the call-chain. find_git_ref is a fundamental utility function in the plugin implementation. This change alone likely will introduce some bugs, possible crashes.
So the usage of find_git_ref has to be checked up to the UI level and it must be ensured the errors get handled.
Summary
find_git_refcatchesExceptionand silently returnsNone. This makes real failures (e.g., repository I/O errors, backend API breakage, permission issues) indistinguishable from a normal "ref not found" result. Callers can then take wrong code paths (e.g., treating a broken repo as missing ref) and continue with invalid state, which is a classic silent-failure bug.Files changed
picard/git/ref_utils.py(modified)Testing
Summary
Problem
Solution
AI Usage
In accordance with the AI use policy portion of the MetaBrainz Contribution Guidelines, the level of AI/LLM use in the development of this Pull Request is:
Action
Additional actions required: