sync/git,tests(feat[update_repo]) Fix checkout error detection, add errored 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)

Author: tony

src/libvcs/sync/git.py

@@ -498,7 +498,10 @@ def update_repo(
 
             # Checkout the remote branch
             try:
-                process = self.cmd.checkout(branch=git_tag)
+                process = self.cmd.checkout(
+                    branch=git_tag,
+                    check_returncode=True,
+                )
             except exc.CommandError as e:
                 self.log.exception("Failed to checkout tag: '%s'", git_tag)
                 result.add_error("checkout", str(e), exception=e)
@@ -546,7 +549,10 @@ def update_repo(
 
         else:
             try:
-                process = self.cmd.checkout(branch=git_tag)
+                process = self.cmd.checkout(
+                    branch=git_tag,
+                    check_returncode=True,
+                )
             except exc.CommandError as e:
                 self.log.exception("Failed to checkout tag: '%s'", git_tag)
                 result.add_error("checkout", str(e), exception=e)

tests/sync/test_git.py

@@ -992,3 +992,36 @@ def test_update_repo_fetch_failure_returns_sync_result(
     assert result.errors[0].step == "fetch"
     assert result.errors[0].exception is not None
     assert isinstance(result.errors[0].exception, exc.CommandError)
+
+
+def test_update_repo_checkout_failure_returns_sync_result(
+    create_git_remote_bare_repo: CreateRepoPytestFixtureFn,
+    tmp_path: pathlib.Path,
+) -> None:
+    """Test that a checkout failure in update_repo() returns SyncResult with error."""
+    git_server = create_git_remote_bare_repo()
+    git_repo = GitSync(
+        path=tmp_path / "myrepo",
+        url=git_server.as_uri(),
+    )
+    git_repo.obtain()
+
+    # Make a commit and push so the 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"])
+
+    # Set rev to a nonexistent branch to trigger a checkout failure
+    git_repo.rev = "nonexistent-branch"
+
+    result = git_repo.update_repo()
+
+    assert isinstance(result, SyncResult)
+    assert result.ok is False
+    assert bool(result) is False
+    assert len(result.errors) > 0
+    assert result.errors[0].step == "checkout"
+    assert result.errors[0].exception is not None
+    assert isinstance(result.errors[0].exception, exc.CommandError)

tests/sync/test_hg.py

@@ -10,6 +10,7 @@
 from libvcs import exc
 from libvcs._internal.run import run
 from libvcs._internal.shortcuts import create_project
+from libvcs.sync.base import SyncResult
 from libvcs.sync.hg import HgSync
 
 if not shutil.which("hg"):
@@ -100,3 +101,31 @@ def test_vulnerability_2022_03_12_command_injection(
     assert not pathlib.Path(
         random_dir / "HELLO",
     ).exists(), "Prevent command injection in hg aliases"
+
+
+def test_update_repo_pull_failure_returns_sync_result(
+    tmp_path: pathlib.Path,
+    projects_path: pathlib.Path,
+    hg_remote_repo: pathlib.Path,
+) -> None:
+    """Test that a deleted remote in update_repo() returns SyncResult with error."""
+    repo_name = "my_hg_error_project"
+
+    hg_repo = HgSync(
+        url=f"file://{hg_remote_repo}",
+        path=projects_path / repo_name,
+    )
+
+    # First update_repo clones since .hg doesn't exist yet
+    hg_repo.update_repo()
+
+    # Delete the remote to cause a pull failure
+    shutil.rmtree(hg_remote_repo)
+
+    result = hg_repo.update_repo()
+
+    assert isinstance(result, SyncResult)
+    assert result.ok is False
+    assert len(result.errors) > 0
+    assert result.errors[0].step == "pull"
+    assert isinstance(result.errors[0].exception, exc.CommandError)

tests/sync/test_svn.py

@@ -7,6 +7,8 @@
 
 import pytest
 
+from libvcs import exc
+from libvcs.sync.base import SyncResult
 from libvcs.sync.svn import SvnSync
 
 if t.TYPE_CHECKING:
@@ -74,3 +76,27 @@ def test_repo_svn_remote_checkout(
     assert svn_repo.get_revision_file("./") == 0
 
     assert svn_repo_checkout_dir.exists()
+
+
+def test_update_repo_checkout_failure_returns_sync_result(
+    create_svn_remote_repo: CreateRepoPytestFixtureFn,
+    tmp_path: pathlib.Path,
+    projects_path: pathlib.Path,
+) -> None:
+    """Test that a deleted remote in update_repo() returns SyncResult with error."""
+    svn_server = create_svn_remote_repo()
+    svn_repo_checkout_dir = projects_path / "my_svn_checkout"
+    svn_repo = SvnSync(path=svn_repo_checkout_dir, url=f"file://{svn_server!s}")
+
+    svn_repo.obtain()
+
+    # Delete the remote to cause an update failure
+    shutil.rmtree(svn_server)
+
+    result = svn_repo.update_repo()
+
+    assert isinstance(result, SyncResult)
+    assert result.ok is False
+    assert len(result.errors) > 0
+    assert result.errors[0].step == "checkout"
+    assert isinstance(result.errors[0].exception, exc.CommandError)