sync: Return SyncResult from update_repo()#514
Conversation
why: GitSync.update_repo() catches CommandError at 10+ locations and silently returns None, making it impossible for callers to detect sync failures. This causes vcspull to report "✓ Synced" even when git operations (fetch, rebase, checkout) fail. what: - Add SyncResult and SyncError dataclasses to sync/base.py - Change BaseSync.update_repo() return type from None to SyncResult - Transform GitSync.update_repo() except handlers to record errors in SyncResult while preserving existing control flow - Update SvnSync and HgSync for API consistency - Add tests for success and fetch-failure scenarios
why: The old `# ruff: NOQA: A002` file-level comment syntax is no longer recognized by ruff (RUF103), causing CI to fail. what: - Remove obsolete `# ruff: NOQA: A002` from subprocess.py - Add per-file-ignores entry in pyproject.toml for A002 on subprocess.py
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #514 +/- ##
==========================================
+ Coverage 53.48% 57.07% +3.58%
==========================================
Files 38 38
Lines 5704 6166 +462
Branches 1062 1069 +7
==========================================
+ Hits 3051 3519 +468
+ Misses 2143 2126 -17
- Partials 510 521 +11 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
…rrored sync tests why: cmd.checkout() without check_returncode=True silently swallowed checkout failures, making the except CommandError block dead code. Also, SVN and HG had no error-path tests for update_repo(). what: - Add check_returncode=True to both cmd.checkout() calls in GitSync.update_repo() - Add test_update_repo_checkout_failure_returns_sync_result for git (nonexistent branch) - Add test_update_repo_checkout_failure_returns_sync_result for svn (deleted remote) - Add test_update_repo_pull_failure_returns_sync_result for hg (deleted remote)
This comment has been minimized.
This comment has been minimized.
why: Code review identified incomplete docstrings missing Parameters and Returns sections required by project standards. what: - Add Parameters/Returns to SyncResult.__bool__() and add_error() - Add Parameters/Returns to GitSync.update_repo() - Add Returns to HgSync.update_repo() - Add Parameters/Returns to SvnSync.update_repo()
why: Stash-save error handler was missing return statement, allowing checkout/rebase to proceed with unstashed changes and stash.pop() to fail on a non-existent stash. what: - Add return result after stash-save error recording
why: CommandError from obtain() propagated uncaught, bypassing the structured SyncResult error reporting that all other sync steps use. what: - Wrap obtain() in try/except in GitSync.update_repo() - Wrap obtain() in try/except in HgSync.update_repo() - Wrap obtain() in try/except in SvnSync.update_repo() - Update command injection test to check SyncResult instead of raised exception
This comment has been minimized.
This comment has been minimized.
why: The rev_list HEAD error handler caught CommandError and returned result without calling add_error(), leaving ok=True on failure. what: - Capture exception and call result.add_error() in rev-list HEAD handler
why: The rev-list HEAD error handler had no test coverage, which allowed the missing add_error() bug to go undetected. what: - Add test_update_repo_rev_list_head_failure_returns_sync_result
…ing paths why: When a git repo has a local branch whose name matches a directory (e.g. branch "notes" + directory "notes/"), git rev-list fails with "fatal: ambiguous argument". The error was caught but not recorded in SyncResult, so vcspull reported the sync as successful despite the visible fatal error. what: - Check show_ref output for refs/heads/<tag> before calling rev-list - Use fully-qualified refs/heads/ path when available, avoiding the ambiguity entirely (no fatal error emitted, no retry needed) - Fall back to bare tag name when no refs/heads/ match exists (e.g. tags, SHAs, or refs not yet fetched)
why: Several error paths in GitSync.update_repo() are not wrapped in try/except, so failures propagate as uncaught exceptions instead of being recorded in SyncResult. This causes vcspull to either crash or report false successes. what: - Add test_update_repo_submodule_failure_recorded (line 589 gap) - Add test_update_repo_symbolic_ref_failure_recorded (line 420-424 gap) - Add test_update_repo_remote_ref_not_found_recorded (line 462 gap) - All marked xfail(strict=True) pending fix
why: Several error paths in GitSync.update_repo() were not wrapped in try/except, causing failures to propagate as uncaught exceptions instead of being recorded in SyncResult. This made vcspull report false successes (e.g. "✓ Synced cpython" when git had a fatal error). what: - Wrap symbolic_ref() in try/except with check_returncode=True (detached HEAD) - Wrap get_current_remote_name() in try/except - Catch GitRemoteRefNotFound instead of bare raise; record in SyncResult - Wrap submodule.update() in try/except (non-fatal: records but doesn't abort) - Use contextlib.suppress for best-effort recovery paths (rebase --abort, etc.) - Fix GitRemoteRefNotFound.__str__ to not require cmd attribute - Add comment explaining intentional rev_list(tag) catch behavior - Update existing test assertions for new check_returncode=True kwarg - Remove xfail from three new error-path tests
Code reviewNo issues found. Checked for bugs and CLAUDE.md compliance. 🤖 Generated with Claude Code |
why: rev is typed str | None, so assigning None needs no suppression. mypy flags the unused comment as an error. what: - Remove unused type: ignore[assignment] on git_repo.rev = None
why: The invalid_upstream handler recorded the error (ok=False) but fell through to stash pop, creating inconsistent state. what: - Add return result after add_error in the invalid_upstream branch
why: The flag processing loop used bare `all` (Python builtin, always truthy) instead of the `_all` parameter, causing --all to be appended unconditionally twice to every rev-list command. what: - Change (all, "--all") to (_all, "--all") at both occurrences - Add test_rev_list_all_parameter to verify _all flag behavior
why: test_update_repo_pull_failure_returns_sync_result deleted the session-scoped hg_remote_repo, breaking all subsequent tests that depend on it (e.g. tests/url/test_hg.py::test_hg_url). what: - Use create_hg_remote_repo factory to create a disposable remote - Matches the pattern used by the equivalent git test
Code reviewNo issues found. Checked for bugs and CLAUDE.md compliance. 🤖 Generated with Claude Code - If this code review was useful, please react with 👍. Otherwise, react with 👎. |
why: Document the changes introduced in #514 for the upcoming release. what: - New feature: update_repo() returns SyncResult across Git, Hg, SVN - Bug fixes: rev_list _all parameter, rev-list disambiguation, rebase early return - Tests: Fix hg test destroying session-scoped fixture
…d base class why: README showed a bare update_repo() call that ignored errors, and BaseSync.update_repo() docstring lacked a Returns section for autodoc. what: - Update README sync example to capture SyncResult and check .ok/.errors - Add NumPy-style Returns section to BaseSync.update_repo() docstring
…otation why: Project style favors explicit class-level annotations over inferred instance attributes. what: - Add `_message: str` annotation to GitRemoteRefNotFound class body
…l package why: Users currently must import from libvcs.sync.base directly. Exporting from __init__.py makes the public API explicit. what: - Add SyncResult and SyncError to libvcs.__init__ imports - Add both to __all__
why: 7 of 13 error steps in GitSync.update_repo() had no test coverage. what: - Add test_update_repo_obtain_failure_recorded - Add test_update_repo_set_remotes_failure_recorded - Add test_update_repo_remote_name_failure_recorded - Add test_update_repo_status_failure_recorded - Add test_update_repo_stash_save_failure_recorded - Add test_update_repo_rebase_invalid_upstream_recorded - Add test_update_repo_rebase_conflict_recorded - Add test_update_repo_stash_pop_failure_recorded - Add test_sync_result_multiple_errors for multi-error accumulation
tony
left a comment
There was a problem hiding this comment.
Code review
No issues found. Checked for bugs and CLAUDE.md compliance.
Reviewed: error handling in sync/git.py, sync/hg.py, sync/svn.py; SyncResult/SyncError dataclasses in sync/base.py; rev_list parameter fix in cmd/git.py; GitRemoteRefNotFound exception refactor; test coverage across 13 error steps; public exports in init.py.
🤖 Generated with Claude Code
- If this code review was useful, please react with 👍. Otherwise, react with 👎.
Summary
update_repo()across all three VCS sync classes (Git, Hg, SVN) now returns aSyncResultinstead ofNone. This lets callers distinguish successful syncs from failures and inspect structured error details — instead of errors being silently swallowed or bubbling as uncaught exceptions.What changed
sync/base.py:SyncResult(tracksokstatus + error list) andSyncError(records step name, message, and original exception)GitSync.update_repo(): All 10+except CommandError: returnpaths now record errors inSyncResultwith labeled steps (fetch,rebase,checkout,stash-save, etc.) and preserve the original early-return vs. continue-on-error control flowHgSync.update_repo()andSvnSync.update_repo(): Updated for API consistency — wrapobtainandpull/updatefailures inSyncResultGitRemoteRefNotFound: Added__str__()override to avoidAttributeErrorwhen the parentCommandError.__str__()expectscmd/returncodeattributes that this subclass doesn't setAdditional fixes included
Git.rev_list(): Fixed_allparameter reference that was shadowed by the builtinallrev-listdisambiguation: Use fully-qualifiedrefs/heads/paths when a local branch name collides with a filesystem pathA002suppression from inline comment toper-file-ignoresinpyproject.tomltest_update_repo_pull_failure_returns_sync_result(hg) now uses thecreate_hg_remote_repofactory instead of deleting the session-scopedhg_remote_repofixtureDesign decisions
SyncResultfollows the structured result pattern. Git'supdate_repo()already caught and discardedCommandErrorat every step — this makes those outcomes inspectable rather than silentfetchfailure returns early (fatal).stash-savefailure continues (non-fatal).rev-list HEADfailure on initial repos returnsok=True(benign — nothing to do)contextlib.suppress(CommandError)for cleanup operations that may fail (rebase abort, stash pop, reset) — matching git's own philosophy of trying cleanup but not failing if it doesn't workTest plan
test_update_repo_success_returns_sync_result— ok=True on successful synctest_update_repo_fetch_failure_returns_sync_result— fetch failure captured in SyncResulttest_update_repo_symbolic_ref_failure_returns_sync_result— symbolic-ref failure pathtest_update_repo_checkout_failure_returns_sync_result— checkout error detectiontest_update_repo_stash_save_failure_returns_sync_result— stash-save failure continuestest_update_repo_rev_list_head_failure_returns_sync_result— rev-list HEAD failure pathtest_update_repo_pull_failure_returns_sync_result(hg) — pull failure with disposable remotetest_update_repo_update_failure_returns_sync_result(svn) — update failure pathtest_rev_list_all_parameter— verifies_allparameter fixCompanion PR
SyncResultand reports failures to users)