sync/git(fix[update_repo]) Wrap unprotected error paths in SyncResult

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

Author: tony

src/libvcs/sync/git.py

@@ -17,6 +17,7 @@
 
 from __future__ import annotations
 
+import contextlib
 import dataclasses
 import logging
 import pathlib
@@ -71,9 +72,14 @@ class GitRemoteRefNotFound(exc.CommandError):
     """Raised when a git remote ref (tag, branch) could not be found."""
 
     def __init__(self, git_tag: str, ref_output: str, *args: object) -> None:
-        return super().__init__(
-            f"Could not fetch remote in refs/remotes/{git_tag}. Output: {ref_output}",
+        self._message = (
+            f"Could not fetch remote in refs/remotes/{git_tag}. Output: {ref_output}"
         )
+        super().__init__(self._message)
+
+    def __str__(self) -> str:
+        """Return descriptive message without requiring cmd attribute."""
+        return self._message
 
 
 @dataclasses.dataclass
@@ -419,8 +425,17 @@ def update_repo(
 
         if not git_tag:
             self.log.debug("No git revision set, defaulting to origin/master")
-            symref = self.cmd.symbolic_ref(name="HEAD", short=True)
-            git_tag = symref.rstrip() if symref else "origin/master"
+            try:
+                symref = self.cmd.symbolic_ref(
+                    name="HEAD",
+                    short=True,
+                    check_returncode=True,
+                )
+                git_tag = symref.rstrip() if symref else "origin/master"
+            except exc.CommandError as e:
+                self.log.exception("Failed to determine current branch")
+                result.add_error("symbolic-ref", str(e), exception=e)
+                return result
         self.log.debug("git_tag: %s", git_tag)
 
         self.log.info("Updating to '%s'.", git_tag)
@@ -448,7 +463,12 @@ def update_repo(
 
         # show-ref output is in the form "<sha> refs/remotes/<remote>/<tag>"
         # we must strip the remote from the tag.
-        git_remote_name = self.get_current_remote_name()
+        try:
+            git_remote_name = self.get_current_remote_name()
+        except (exc.CommandError, GitNoBranchFound, GitRemoteSetError) as e:
+            self.log.exception("Failed to determine remote name")
+            result.add_error("remote-name", str(e), exception=e)
+            return result
 
         if f"refs/remotes/{git_tag}" in show_ref_output:
             m = re.match(
@@ -459,7 +479,17 @@ def update_repo(
                 re.MULTILINE,
             )
             if m is None:
-                raise GitRemoteRefNotFound(git_tag=git_tag, ref_output=show_ref_output)
+                ref_err = GitRemoteRefNotFound(
+                    git_tag=git_tag,
+                    ref_output=show_ref_output,
+                )
+                self.log.error("Remote ref not found: '%s'", git_tag)
+                result.add_error(
+                    "remote-ref-not-found",
+                    str(ref_err),
+                    exception=ref_err,
+                )
+                return result
             git_remote_name = m.group("git_remote_name")
             git_tag = m.group("git_tag")
         self.log.debug("git_remote_name: %s", git_remote_name)
@@ -485,6 +515,10 @@ def update_repo(
             )
 
         except exc.CommandError as e:
+            # Intentionally not recorded in SyncResult: the ref may not be
+            # fetched yet.  The error_code drives the fetch-then-checkout
+            # logic below.  Ambiguity errors are prevented by the
+            # refs/heads/ disambiguation above.
             error_code = e.returncode if e.returncode is not None else 0
             tag_sha = ""
         self.log.debug("tag_sha: %s", tag_sha)
@@ -544,9 +578,11 @@ def update_repo(
                     result.add_error("rebase", str(e), exception=e)
                 else:
                     # Rebase failed: Restore previous state.
-                    self.cmd.rebase(abort=True)
+                    with contextlib.suppress(exc.CommandError):
+                        self.cmd.rebase(abort=True)
                     if need_stash:
-                        self.cmd.stash.pop(index=True, quiet=True)
+                        with contextlib.suppress(exc.CommandError):
+                            self.cmd.stash.pop(index=True, quiet=True)
 
                     self.log.exception(
                         f"\nFailed to rebase in: '{self.path}'.\n"
@@ -560,13 +596,20 @@ def update_repo(
                     process = self.cmd.stash.pop(index=True, quiet=True)
                 except exc.CommandError:
                     # Stash pop --index failed: Try again dropping the index
-                    self.cmd.reset(hard=True, quiet=True)
+                    with contextlib.suppress(exc.CommandError):
+                        self.cmd.reset(hard=True, quiet=True)
                     try:
                         process = self.cmd.stash.pop(quiet=True)
                     except exc.CommandError as e:
                         # Stash pop failed: Restore previous state.
-                        self.cmd.reset(pathspec=head_sha, hard=True, quiet=True)
-                        self.cmd.stash.pop(index=True, quiet=True)
+                        with contextlib.suppress(exc.CommandError):
+                            self.cmd.reset(
+                                pathspec=head_sha,
+                                hard=True,
+                                quiet=True,
+                            )
+                        with contextlib.suppress(exc.CommandError):
+                            self.cmd.stash.pop(index=True, quiet=True)
                         self.log.exception(
                             f"\nFailed to rebase in: '{self.path}'.\n"
                             "You will have to resolve the "
@@ -586,7 +629,11 @@ def update_repo(
                 result.add_error("checkout", str(e), exception=e)
                 return result
 
-        self.cmd.submodule.update(recursive=True, init=True, log_in_real_time=True)
+        try:
+            self.cmd.submodule.update(recursive=True, init=True, log_in_real_time=True)
+        except exc.CommandError as e:
+            self.log.exception("Failed to update submodules")
+            result.add_error("submodule-update", str(e), exception=e)
         return result
 
     def remotes(self) -> GitSyncRemoteDict:

tests/sync/test_git.py

@@ -154,14 +154,17 @@ def test_repo_update_handle_cases(
     cmd_mock = mocker.spy(git_repo.cmd, "symbolic_ref")
     git_repo.update_repo()
 
-    cmd_mock.assert_any_call(name="HEAD", short=True)
+    cmd_mock.assert_any_call(name="HEAD", short=True, check_returncode=True)
 
     cmd_mock.reset_mock()
 
     # will only look up symbolic-ref if no rev specified for object
     git_repo.rev = "HEAD"
     git_repo.update_repo()
-    assert mocker.call(name="HEAD", short=True) not in cmd_mock.mock_calls
+    assert (
+        mocker.call(name="HEAD", short=True, check_returncode=True)
+        not in cmd_mock.mock_calls
+    )
 
 
 @pytest.mark.parametrize(
@@ -225,7 +228,7 @@ def test_repo_update_stash_cases(
     cmd_mock = mocker.spy(git_repo.cmd, "symbolic_ref")
     git_repo.update_repo()
 
-    cmd_mock.assert_any_call(name="HEAD", short=True)
+    cmd_mock.assert_any_call(name="HEAD", short=True, check_returncode=True)
 
 
 @pytest.mark.parametrize(
@@ -1061,17 +1064,17 @@ def test_update_repo_rev_list_head_failure_returns_sync_result(
     assert isinstance(result.errors[0].exception, exc.CommandError)
 
 
-@pytest.mark.xfail(strict=True, reason="submodule.update not wrapped in try/except")
 def test_update_repo_submodule_failure_recorded(
     create_git_remote_bare_repo: CreateRepoPytestFixtureFn,
     tmp_path: pathlib.Path,
+    mocker: MockerFixture,
 ) -> None:
     """Test that submodule.update() failure is recorded in SyncResult.
 
-    When .gitmodules references a non-existent submodule URL, the
-    ``git submodule update`` call in update_repo() fails.  Currently
-    this is not wrapped in try/except, so the exception propagates
-    instead of being recorded in SyncResult.
+    When ``git submodule update`` fails, the error should be recorded
+    in SyncResult rather than propagating as an uncaught exception.
+    We mock the submodule.update call to raise CommandError since
+    triggering a real submodule failure is git-version-dependent.
     """
     git_server = create_git_remote_bare_repo()
     git_repo = GitSync(
@@ -1080,26 +1083,32 @@ def test_update_repo_submodule_failure_recorded(
     )
     git_repo.obtain()
 
-    # Make an initial commit and push so update_repo has a valid HEAD
+    # Make a commit and push so update_repo has a valid HEAD
     initial_file = git_repo.path / "initial_file"
     initial_file.write_text("content", encoding="utf-8")
     git_repo.run(["add", str(initial_file)])
     git_repo.run(["commit", "-m", "initial commit"])
     git_repo.run(["push"])
 
-    # Add a .gitmodules file that references a non-existent submodule URL
-    gitmodules = git_repo.path / ".gitmodules"
-    gitmodules.write_text(
-        '[submodule "broken"]\n\tpath = broken\n\turl = file:///nonexistent/repo.git\n',
-        encoding="utf-8",
-    )
-    git_repo.run(["add", ".gitmodules"])
-    git_repo.run(["commit", "-m", "add broken submodule ref"])
+    # Make another commit, push, then reset to create a "behind" state
+    another_file = git_repo.path / "another_file"
+    another_file.write_text("more content", encoding="utf-8")
+    git_repo.run(["add", str(another_file)])
+    git_repo.run(["commit", "-m", "second commit"])
     git_repo.run(["push"])
-
-    # Reset behind so update_repo triggers a fetch+checkout cycle
     git_repo.run(["reset", "--hard", "HEAD^"])
 
+    # Mock submodule.update to raise CommandError
+    mocker.patch.object(
+        git_repo.cmd.submodule,
+        "update",
+        side_effect=exc.CommandError(
+            output="fatal: clone of 'file:///nonexistent' failed",
+            returncode=128,
+            cmd="git submodule update --init --recursive",
+        ),
+    )
+
     result = git_repo.update_repo()
 
     assert isinstance(result, SyncResult)
@@ -1108,7 +1117,6 @@ def test_update_repo_submodule_failure_recorded(
     assert any(e.step == "submodule-update" for e in result.errors)
 
 
-@pytest.mark.xfail(strict=True, reason="symbolic_ref not wrapped in try/except")
 def test_update_repo_symbolic_ref_failure_recorded(
     create_git_remote_bare_repo: CreateRepoPytestFixtureFn,
     tmp_path: pathlib.Path,
@@ -1149,10 +1157,6 @@ def test_update_repo_symbolic_ref_failure_recorded(
     assert any(e.step == "symbolic-ref" for e in result.errors)
 
 
-@pytest.mark.xfail(
-    strict=True,
-    reason="GitRemoteRefNotFound raised instead of recorded in SyncResult",
-)
 def test_update_repo_remote_ref_not_found_recorded(
     create_git_remote_bare_repo: CreateRepoPytestFixtureFn,
     tmp_path: pathlib.Path,