diff --git a/CHANGES b/CHANGES index e7d8fce33..896120bf4 100644 --- a/CHANGES +++ b/CHANGES @@ -20,6 +20,43 @@ $ uv add libvcs --prerelease allow _Notes on the upcoming release will go here._ +### New features + +#### sync: Return {class}`~libvcs.sync.base.SyncResult` from `update_repo()` (#514) + +`update_repo()` across {class}`~libvcs.sync.git.GitSync`, +{class}`~libvcs.sync.hg.HgSync`, and {class}`~libvcs.sync.svn.SvnSync` +now returns a {class}`~libvcs.sync.base.SyncResult` instead of `None`. +Callers can inspect `result.ok` and `result.errors` to distinguish +successful syncs from failures. + +- New dataclasses: {class}`~libvcs.sync.base.SyncResult` and + {class}`~libvcs.sync.base.SyncError` +- Git: 10+ silent `except CommandError: return` paths now record + structured errors with labeled steps (`fetch`, `rebase`, `checkout`, + `stash-save`, etc.) +- Hg and SVN: Wrap `obtain` and `pull`/`update` failures for API + consistency + +Companion change: [vcspull#512](https://github.com/vcs-python/vcspull/pull/512) + +### Bug Fixes + +- cmd: Fix `Git.rev_list()` referencing builtin `all` instead of `_all` + parameter (#514) + +- sync: Disambiguate `rev-list` when a local branch name collides with a + filesystem path by using fully-qualified `refs/heads/` refs (#514) + +- sync: Return early with error on invalid-upstream rebase instead of + falling through to stash-pop (#514) + +### Tests + +- sync: Fix `test_update_repo_pull_failure_returns_sync_result` (hg) + destroying the session-scoped `hg_remote_repo` fixture, which broke + downstream tests like `test_hg_url` (#514) + ## libvcs 0.38.6 (2026-01-27) ### Bug Fixes diff --git a/README.md b/README.md index 42d7d120c..0445daf50 100644 --- a/README.md +++ b/README.md @@ -70,9 +70,13 @@ repo = GitSync( ) # Clone (if not exists) or fetch & update (if exists) -repo.update_repo() +result = repo.update_repo() -print(f"Current revision: {repo.get_revision()}") +if result.ok: + print(f"Current revision: {repo.get_revision()}") +else: + for error in result.errors: + print(f"Sync failed at {error.step}: {error.message}") ``` ### 2. Command Abstraction diff --git a/pyproject.toml b/pyproject.toml index 6f17df6d0..dee794001 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -211,6 +211,7 @@ convention = "numpy" [tool.ruff.lint.per-file-ignores] "*/__init__.py" = ["F401"] +"src/libvcs/_internal/subprocess.py" = ["A002"] [tool.pytest.ini_options] addopts = [ diff --git a/src/libvcs/__init__.py b/src/libvcs/__init__.py index c19cc0923..0d2864ce5 100644 --- a/src/libvcs/__init__.py +++ b/src/libvcs/__init__.py @@ -5,7 +5,7 @@ import logging from ._internal.run import CmdLoggingAdapter -from .sync.base import BaseSync +from .sync.base import BaseSync, SyncError, SyncResult from .sync.git import GitSync from .sync.hg import HgSync from .sync.svn import SvnSync @@ -16,6 +16,8 @@ "GitSync", "HgSync", "SvnSync", + "SyncError", + "SyncResult", ] logger = logging.getLogger(__name__) diff --git a/src/libvcs/_internal/subprocess.py b/src/libvcs/_internal/subprocess.py index 003de28f2..3e73f6461 100644 --- a/src/libvcs/_internal/subprocess.py +++ b/src/libvcs/_internal/subprocess.py @@ -1,4 +1,3 @@ -# ruff: NOQA: A002 r"""Invocable :mod:`subprocess` wrapper. Defer running a subprocess, such as by handing to an executor. diff --git a/src/libvcs/cmd/git.py b/src/libvcs/cmd/git.py index 23fd21d25..d8a9b38d0 100644 --- a/src/libvcs/cmd/git.py +++ b/src/libvcs/cmd/git.py @@ -2193,7 +2193,7 @@ def rev_list( for flag, shell_flag in [ # Limiting output - (all, "--all"), + (_all, "--all"), (author, "--author"), (committer, "--committer"), (grep, "--grep"), @@ -2212,7 +2212,7 @@ def rev_list( (first_parent, "--first-parent"), (exclude_first_parent_only, "--exclude-first-parent-only"), (_not, "--not"), - (all, "--all"), + (_all, "--all"), (exclude, "--exclude"), (reflog, "--reflog"), (alternative_refs, "--alternative-refs"), diff --git a/src/libvcs/sync/base.py b/src/libvcs/sync/base.py index c026ad5d2..6bf947860 100644 --- a/src/libvcs/sync/base.py +++ b/src/libvcs/sync/base.py @@ -2,6 +2,7 @@ from __future__ import annotations +import dataclasses import logging import pathlib import typing as t @@ -14,6 +15,82 @@ logger = logging.getLogger(__name__) +@dataclasses.dataclass +class SyncError: + """An error encountered during a sync step. + + Examples + -------- + >>> error = SyncError(step="fetch", message="remote not found") + >>> error.step + 'fetch' + >>> error.message + 'remote not found' + >>> error.exception is None + True + """ + + step: str + message: str + exception: Exception | None = None + + +@dataclasses.dataclass +class SyncResult: + """Result of a repository synchronization. + + Examples + -------- + >>> result = SyncResult() + >>> result.ok + True + >>> bool(result) + True + + >>> result = SyncResult() + >>> result.add_error(step="fetch", message="remote not found") + >>> result.ok + False + >>> bool(result) + False + >>> result.errors[0].step + 'fetch' + """ + + ok: bool = True + errors: list[SyncError] = dataclasses.field(default_factory=list) + + def __bool__(self) -> bool: + """Return True if the sync succeeded without errors. + + Returns + ------- + bool + True if no errors were recorded, False otherwise. + """ + return self.ok + + def add_error( + self, + step: str, + message: str, + exception: Exception | None = None, + ) -> None: + """Record an error and mark the result as failed. + + Parameters + ---------- + step : str + Name of the sync step that failed (e.g. ``"fetch"``, ``"checkout"``). + message : str + Human-readable description of the error. + exception : Exception or None, optional + The underlying exception, if available. + """ + self.ok = False + self.errors.append(SyncError(step=step, message=message, exception=exception)) + + class VCSLocation(t.NamedTuple): """Generic VCS Location (URL and optional revision).""" @@ -200,8 +277,14 @@ def ensure_dir(self, *args: t.Any, **kwargs: t.Any) -> bool: return True - def update_repo(self, *args: t.Any, **kwargs: t.Any) -> None: - """Pull latest changes to here from remote repository.""" + def update_repo(self, *args: t.Any, **kwargs: t.Any) -> SyncResult: + """Pull latest changes to here from remote repository. + + Returns + ------- + SyncResult + Result of the sync operation, with any errors recorded. + """ raise NotImplementedError def obtain(self, *args: t.Any, **kwargs: t.Any) -> None: diff --git a/src/libvcs/sync/git.py b/src/libvcs/sync/git.py index 4916c6bf7..59dee61be 100644 --- a/src/libvcs/sync/git.py +++ b/src/libvcs/sync/git.py @@ -17,6 +17,7 @@ from __future__ import annotations +import contextlib import dataclasses import logging import pathlib @@ -29,6 +30,7 @@ from libvcs.cmd.git import Git from libvcs.sync.base import ( BaseSync, + SyncResult, VCSLocation, convert_pip_url as base_convert_pip_url, ) @@ -69,10 +71,17 @@ def __init__(self, *args: object) -> None: class GitRemoteRefNotFound(exc.CommandError): """Raised when a git remote ref (tag, branch) could not be found.""" + _message: str + 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 @@ -380,25 +389,55 @@ def update_repo( set_remotes: bool = False, *args: t.Any, **kwargs: t.Any, - ) -> None: - """Pull latest changes from git remote.""" + ) -> SyncResult: + """Pull latest changes from git remote. + + Parameters + ---------- + set_remotes : bool + If True, configure remotes before updating. + + Returns + ------- + SyncResult + Result of the sync operation, with any errors recorded. + """ + result = SyncResult() self.ensure_dir() if not pathlib.Path(self.path / ".git").is_dir(): - self.obtain() - self.update_repo(set_remotes=set_remotes) - return + try: + self.obtain() + except exc.CommandError as e: + self.log.exception("Failed to obtain repository") + result.add_error("obtain", str(e), exception=e) + return result + return self.update_repo(set_remotes=set_remotes) if set_remotes: - self.set_remotes(overwrite=True) + try: + self.set_remotes(overwrite=True) + except exc.CommandError as e: + self.log.exception("Failed to set remotes") + result.add_error("set-remotes", str(e), exception=e) + return result # Get requested revision or tag url, git_tag = self.url, getattr(self, "rev", None) 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) @@ -410,9 +449,10 @@ def update_repo( max_count=1, check_returncode=True, ) - except exc.CommandError: + except exc.CommandError as e: self.log.exception("Failed to get the hash for HEAD") - return + result.add_error("rev-list-head", str(e), exception=e) + return result self.log.debug("head_sha: %s", head_sha) @@ -425,7 +465,12 @@ def update_repo( # show-ref output is in the form " refs/remotes//" # 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( @@ -436,7 +481,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) @@ -444,14 +499,28 @@ def update_repo( # This will fail if the tag does not exist (it probably has not # been fetched yet). + # + # When the ref is local, use the fully-qualified refs/heads/ path + # if available to avoid ambiguity with paths (e.g. a branch named + # "notes" when a directory "notes/" also exists). + if is_remote_ref: + rev_list_commit = git_remote_name + "/" + git_tag + elif f"refs/heads/{git_tag}" in show_ref_output: + rev_list_commit = f"refs/heads/{git_tag}" + else: + rev_list_commit = git_tag try: error_code = 0 tag_sha = self.cmd.rev_list( - commit=git_remote_name + "/" + git_tag if is_remote_ref else git_tag, + commit=rev_list_commit, max_count=1, ) 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) @@ -460,21 +529,23 @@ def update_repo( somethings_up = (error_code, is_remote_ref, tag_sha != head_sha) if all(not x for x in somethings_up): self.log.info("Already up-to-date.") - return + return result try: process = self.cmd.fetch(log_in_real_time=True, check_returncode=True) - except exc.CommandError: + except exc.CommandError as e: self.log.exception("Failed to fetch repository '%s'", url) - return + result.add_error("fetch", str(e), exception=e) + return result if is_remote_ref: # Check if stash is needed try: process = self.cmd.status(porcelain=True, untracked_files="no") - except exc.CommandError: + except exc.CommandError as e: self.log.exception("Failed to get the status") - return + result.add_error("status", str(e), exception=e) + return result need_stash = len(process) > 0 # If not in clean state, stash changes in order to be able @@ -484,15 +555,21 @@ def update_repo( git_stash_save_options = "--quiet" try: process = self.cmd.stash.save(message=git_stash_save_options) - except exc.CommandError: + except exc.CommandError as e: self.log.exception("Failed to stash changes") + result.add_error("stash-save", str(e), exception=e) + return result # Checkout the remote branch try: - process = self.cmd.checkout(branch=git_tag) - except exc.CommandError: + 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) - return + result.add_error("checkout", str(e), exception=e) + return result # Rebase changes from the remote branch try: @@ -500,45 +577,67 @@ def update_repo( except exc.CommandError as e: if any(msg in str(e) for msg in ["invalid_upstream", "Aborting"]): self.log.exception("Invalid upstream remote. Rebase aborted.") + result.add_error("rebase", str(e), exception=e) + return result 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" "You will have to resolve the conflicts manually", ) - return + result.add_error("rebase", str(e), exception=e) + return result if need_stash: try: 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: + 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 " "conflicts manually", ) - return + result.add_error("stash-pop", str(e), exception=e) + return result else: try: - process = self.cmd.checkout(branch=git_tag) - except exc.CommandError: + 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) - return + 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: """Return remotes like git remote -v. diff --git a/src/libvcs/sync/hg.py b/src/libvcs/sync/hg.py index 0085fb480..18e0e0108 100644 --- a/src/libvcs/sync/hg.py +++ b/src/libvcs/sync/hg.py @@ -15,10 +15,11 @@ import pathlib import typing as t +from libvcs import exc from libvcs._internal.types import StrPath from libvcs.cmd.hg import Hg -from .base import BaseSync +from .base import BaseSync, SyncResult logger = logging.getLogger(__name__) @@ -64,11 +65,27 @@ def get_revision(self) -> str: """Get latest revision of this mercurial repository.""" return self.run(["parents", "--template={rev}"]) - def update_repo(self, *args: t.Any, **kwargs: t.Any) -> None: - """Pull changes from remote Mercurial repository into this one.""" + def update_repo(self, *args: t.Any, **kwargs: t.Any) -> SyncResult: + """Pull changes from remote Mercurial repository into this one. + + Returns + ------- + SyncResult + Result of the sync operation, with any errors recorded. + """ + result = SyncResult() if not pathlib.Path(self.path / ".hg").exists(): - self.obtain() - self.update_repo() + try: + self.obtain() + except exc.CommandError as e: + self.log.exception("Failed to obtain repository") + result.add_error("obtain", str(e), exception=e) + return result + return self.update_repo() else: - self.cmd.update() - self.cmd.pull(update=True) + try: + self.cmd.update() + self.cmd.pull(update=True) + except exc.CommandError as e: + result.add_error("pull", str(e), exception=e) + return result diff --git a/src/libvcs/sync/svn.py b/src/libvcs/sync/svn.py index 12c393711..08f6c5008 100644 --- a/src/libvcs/sync/svn.py +++ b/src/libvcs/sync/svn.py @@ -21,10 +21,11 @@ import re import typing as t +from libvcs import exc from libvcs._internal.types import StrPath from libvcs.cmd.svn import Svn -from .base import BaseSync +from .base import BaseSync, SyncResult logger = logging.getLogger(__name__) @@ -148,22 +149,43 @@ def update_repo( dest: str | None = None, *args: t.Any, **kwargs: t.Any, - ) -> None: - """Fetch changes from SVN repository to local working copy.""" + ) -> SyncResult: + """Fetch changes from SVN repository to local working copy. + + Parameters + ---------- + dest : str or None, optional + Destination path override for the working copy. + + Returns + ------- + SyncResult + Result of the sync operation, with any errors recorded. + """ + result = SyncResult() self.ensure_dir() if pathlib.Path(self.path / ".svn").exists(): - self.cmd.checkout( - url=self.url, - username=self.username, - password=self.password, - non_interactive=True, - quiet=True, - check_returncode=True, - **kwargs, - ) + try: + self.cmd.checkout( + url=self.url, + username=self.username, + password=self.password, + non_interactive=True, + quiet=True, + check_returncode=True, + **kwargs, + ) + except exc.CommandError as e: + result.add_error("checkout", str(e), exception=e) else: - self.obtain() - self.update_repo() + try: + self.obtain() + except exc.CommandError as e: + self.log.exception("Failed to obtain repository") + result.add_error("obtain", str(e), exception=e) + return result + return self.update_repo() + return result @classmethod def _get_svn_url_rev(cls, location: str) -> tuple[str | None, int]: diff --git a/tests/cmd/test_git.py b/tests/cmd/test_git.py index 0dfe46baa..4f501e0cd 100644 --- a/tests/cmd/test_git.py +++ b/tests/cmd/test_git.py @@ -2625,3 +2625,22 @@ def test_submodule_entry_absorbgitdirs( # Should succeed (empty string or info message) assert result == "" or isinstance(result, str) + + +def test_rev_list_all_parameter(git_repo: GitSync) -> None: + """Test that _all parameter controls --all flag in rev-list.""" + # Create a branch with an extra commit not reachable from master + git_repo.cmd.run(["checkout", "-b", "other-branch"]) + git_repo.cmd.run(["commit", "--allow-empty", "-m", "other-branch-commit"]) + git_repo.cmd.run(["checkout", "master"]) + + # Without _all: only commits reachable from HEAD (master) + result_no_all = git_repo.cmd.rev_list(commit="HEAD") + count_no_all = len(result_no_all.strip().split("\n")) + + # With _all=True: includes commits from all branches + result_with_all = git_repo.cmd.rev_list(commit="HEAD", _all=True) + count_with_all = len(result_with_all.strip().split("\n")) + + # _all=True should return strictly more commits (the other-branch commit) + assert count_with_all > count_no_all diff --git a/tests/sync/test_git.py b/tests/sync/test_git.py index b2794ad9b..61b8584e2 100644 --- a/tests/sync/test_git.py +++ b/tests/sync/test_git.py @@ -15,6 +15,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.git import ( GitRemote, GitStatus, @@ -153,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( @@ -224,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( @@ -923,3 +927,742 @@ def test_repo_git_remote_checkout( assert git_repo_checkout_dir.exists() assert pathlib.Path(git_repo_checkout_dir / ".git").exists() + + +def test_update_repo_success_returns_sync_result( + create_git_remote_bare_repo: CreateRepoPytestFixtureFn, + tmp_path: pathlib.Path, +) -> None: + """Test that a successful update_repo() returns SyncResult with ok=True.""" + git_server = create_git_remote_bare_repo() + git_repo = GitSync( + path=tmp_path / "myrepo", + url=git_server.as_uri(), + ) + git_repo.obtain() + + # Make an initial commit so rev-list HEAD succeeds + 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"]) + + result = git_repo.update_repo() + + assert isinstance(result, SyncResult) + assert result.ok is True + assert result.errors == [] + assert bool(result) is True + + +def test_update_repo_fetch_failure_returns_sync_result( + create_git_remote_bare_repo: CreateRepoPytestFixtureFn, + tmp_path: pathlib.Path, +) -> None: + """Test that a fetch 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", "a commit"]) + git_repo.run(["push"]) + + # 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"]) + git_repo.run(["reset", "--hard", "HEAD^"]) + + # Delete the remote directory to cause a fetch failure + shutil.rmtree(git_server) + + 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 == "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) + + +def test_update_repo_rev_list_head_failure_returns_sync_result( + create_git_remote_bare_repo: CreateRepoPytestFixtureFn, + tmp_path: pathlib.Path, +) -> None: + """update_repo() records rev-list HEAD failure in SyncResult.""" + 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"]) + + # Corrupt HEAD so rev-list HEAD fails + head_file = git_repo.path / ".git" / "HEAD" + head_file.write_text("ref: refs/heads/nonexistent\n") + + 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 == "rev-list-head" + assert result.errors[0].exception is not None + assert isinstance(result.errors[0].exception, exc.CommandError) + + +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 ``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( + path=tmp_path / "myrepo", + url=git_server.as_uri(), + ) + git_repo.obtain() + + # 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"]) + + # 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"]) + 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) + assert result.ok is False + assert len(result.errors) > 0 + assert any(e.step == "submodule-update" for e in result.errors) + + +def test_update_repo_symbolic_ref_failure_recorded( + create_git_remote_bare_repo: CreateRepoPytestFixtureFn, + tmp_path: pathlib.Path, +) -> None: + """Test that symbolic_ref failure on detached HEAD is recorded in SyncResult. + + When a repo is in detached HEAD state and no ``rev`` is set, + ``symbolic_ref --short HEAD`` fails. Currently this is not wrapped + in try/except, so the exception propagates instead of being + recorded in SyncResult. + """ + 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"]) + + # Detach HEAD — symbolic_ref will fail + head_sha = git_repo.run(["rev-parse", "HEAD"]).strip() + git_repo.run(["checkout", head_sha]) + + # Ensure no rev is set so the code path hits symbolic_ref + git_repo.rev = None + + result = git_repo.update_repo() + + assert isinstance(result, SyncResult) + assert result.ok is False + assert len(result.errors) > 0 + assert any(e.step == "symbolic-ref" for e in result.errors) + + +def test_update_repo_remote_ref_not_found_recorded( + create_git_remote_bare_repo: CreateRepoPytestFixtureFn, + tmp_path: pathlib.Path, +) -> None: + """Test that GitRemoteRefNotFound is caught and recorded in SyncResult. + + When show-ref output contains ``refs/remotes/`` but the regex + match fails, ``GitRemoteRefNotFound`` is raised with a bare ``raise``. + It should instead be caught and recorded in SyncResult. + """ + from unittest.mock import patch + + 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 so symbolic_ref is skipped + git_repo.rev = "master" + + # Patch show_ref to return output that contains "refs/remotes/master" + # but in a format that the regex won't match, triggering + # GitRemoteRefNotFound + malformed_show_ref = "not-a-sha refs/remotes/master" + with patch.object(git_repo.cmd, "show_ref", return_value=malformed_show_ref): + result = git_repo.update_repo() + + assert isinstance(result, SyncResult) + assert result.ok is False + assert len(result.errors) > 0 + assert any(e.step == "remote-ref-not-found" for e in result.errors) + + +def test_update_repo_obtain_failure_recorded( + create_git_remote_bare_repo: CreateRepoPytestFixtureFn, + tmp_path: pathlib.Path, + mocker: MockerFixture, +) -> None: + """Test that obtain failure when .git is missing is recorded in SyncResult. + + When .git does not exist, update_repo() calls obtain(). If the clone + fails, the error is recorded as ``obtain``. + """ + git_server = create_git_remote_bare_repo() + git_repo = GitSync( + path=tmp_path / "myrepo", + url=git_server.as_uri(), + ) + + # Mock obtain to raise CommandError (simulates a clone failure) + mocker.patch.object( + git_repo, + "obtain", + side_effect=exc.CommandError( + output="fatal: repository not found", + returncode=128, + cmd="git clone", + ), + ) + + result = git_repo.update_repo() + + assert isinstance(result, SyncResult) + assert result.ok is False + assert len(result.errors) > 0 + assert result.errors[0].step == "obtain" + assert result.errors[0].exception is not None + assert isinstance(result.errors[0].exception, exc.CommandError) + + +def test_update_repo_set_remotes_failure_recorded( + create_git_remote_bare_repo: CreateRepoPytestFixtureFn, + tmp_path: pathlib.Path, + mocker: MockerFixture, +) -> None: + """Test that set_remotes failure is recorded in SyncResult. + + When ``set_remotes=True`` is passed and ``set_remotes()`` raises + CommandError, the error is recorded as ``set-remotes``. + """ + 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"]) + + mocker.patch.object( + git_repo, + "set_remotes", + side_effect=exc.CommandError( + output="fatal: remote error", + returncode=1, + cmd="git remote set-url", + ), + ) + + result = git_repo.update_repo(set_remotes=True) + + assert isinstance(result, SyncResult) + assert result.ok is False + assert len(result.errors) > 0 + assert result.errors[0].step == "set-remotes" + assert result.errors[0].exception is not None + assert isinstance(result.errors[0].exception, exc.CommandError) + + +def test_update_repo_remote_name_failure_recorded( + create_git_remote_bare_repo: CreateRepoPytestFixtureFn, + tmp_path: pathlib.Path, + mocker: MockerFixture, +) -> None: + """Test that get_current_remote_name failure is recorded in SyncResult. + + When ``get_current_remote_name()`` raises, the error is recorded + as ``remote-name``. + """ + 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"]) + + mocker.patch.object( + git_repo, + "get_current_remote_name", + side_effect=exc.CommandError( + output="fatal: could not determine remote", + returncode=1, + cmd="git status", + ), + ) + + result = git_repo.update_repo() + + assert isinstance(result, SyncResult) + assert result.ok is False + assert len(result.errors) > 0 + assert result.errors[0].step == "remote-name" + assert result.errors[0].exception is not None + + +def test_update_repo_status_failure_recorded( + create_git_remote_bare_repo: CreateRepoPytestFixtureFn, + tmp_path: pathlib.Path, + mocker: MockerFixture, +) -> None: + """Test that cmd.status failure is recorded in SyncResult. + + When ``cmd.status()`` raises during the dirty-tree check on a remote + ref, the error is recorded as ``status``. + """ + 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, push, then reset to create a "behind" state + 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"]) + + 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"]) + git_repo.run(["reset", "--hard", "HEAD^"]) + + # cmd.status is called twice: once for get_current_remote_name + # (with short=True, branch=True) and once for the dirty-tree check + # (with porcelain=True, untracked_files="no"). Only the second should fail. + real_status = git_repo.cmd.status + + def status_side_effect(**kwargs: t.Any) -> str: + if "untracked_files" in kwargs: + raise exc.CommandError( + output="fatal: status failed", + returncode=128, + cmd="git status", + ) + return real_status(**kwargs) + + mocker.patch.object( + git_repo.cmd, + "status", + side_effect=status_side_effect, + ) + + result = git_repo.update_repo() + + assert isinstance(result, SyncResult) + assert result.ok is False + assert len(result.errors) > 0 + assert result.errors[0].step == "status" + assert result.errors[0].exception is not None + assert isinstance(result.errors[0].exception, exc.CommandError) + + +def test_update_repo_stash_save_failure_recorded( + create_git_remote_bare_repo: CreateRepoPytestFixtureFn, + tmp_path: pathlib.Path, + mocker: MockerFixture, +) -> None: + """Test that stash save failure is recorded in SyncResult. + + When ``cmd.stash.save()`` raises on a dirty working tree, the error + is recorded as ``stash-save``. + """ + 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, push, then reset to create a "behind" state + 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"]) + + 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"]) + git_repo.run(["reset", "--hard", "HEAD^"]) + + # cmd.status is called twice: first for get_current_remote_name, then + # for the dirty-tree check. Only the second should return dirty. + real_status = git_repo.cmd.status + + def status_side_effect(**kwargs: t.Any) -> str: + if "untracked_files" in kwargs: + return "M some_file" + return real_status(**kwargs) + + mocker.patch.object( + git_repo.cmd, + "status", + side_effect=status_side_effect, + ) + + mocker.patch.object( + git_repo.cmd.stash, + "save", + side_effect=exc.CommandError( + output="fatal: stash save failed", + returncode=1, + cmd="git stash save", + ), + ) + + result = git_repo.update_repo() + + assert isinstance(result, SyncResult) + assert result.ok is False + assert len(result.errors) > 0 + assert result.errors[0].step == "stash-save" + assert result.errors[0].exception is not None + assert isinstance(result.errors[0].exception, exc.CommandError) + + +def test_update_repo_rebase_invalid_upstream_recorded( + create_git_remote_bare_repo: CreateRepoPytestFixtureFn, + tmp_path: pathlib.Path, + mocker: MockerFixture, +) -> None: + """Test that rebase with invalid upstream is recorded in SyncResult. + + When ``cmd.rebase()`` raises and the error message contains + ``invalid_upstream``, the error is recorded as ``rebase`` and the + function returns immediately. + """ + 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, push, then reset to create a "behind" state + 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"]) + + 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"]) + git_repo.run(["reset", "--hard", "HEAD^"]) + + # cmd.status is called twice: first for get_current_remote_name, then + # for the dirty-tree check. Only the second should return dirty. + real_status = git_repo.cmd.status + + def status_side_effect(**kwargs: t.Any) -> str: + if "untracked_files" in kwargs: + return "M some_file" + return real_status(**kwargs) + + mocker.patch.object( + git_repo.cmd, + "status", + side_effect=status_side_effect, + ) + + mocker.patch.object( + git_repo.cmd, + "rebase", + side_effect=exc.CommandError( + output="fatal: invalid_upstream 'origin/master'", + returncode=128, + cmd="git rebase", + ), + ) + + result = git_repo.update_repo() + + assert isinstance(result, SyncResult) + assert result.ok is False + assert len(result.errors) > 0 + assert result.errors[0].step == "rebase" + assert "invalid_upstream" in result.errors[0].message + assert result.errors[0].exception is not None + assert isinstance(result.errors[0].exception, exc.CommandError) + + +def test_update_repo_rebase_conflict_recorded( + create_git_remote_bare_repo: CreateRepoPytestFixtureFn, + tmp_path: pathlib.Path, + mocker: MockerFixture, +) -> None: + """Test that rebase conflict (non-abort) is recorded in SyncResult. + + When ``cmd.rebase()`` raises and the error does NOT contain + ``invalid_upstream`` or ``Aborting``, the function aborts the rebase, + tries to restore stash, and records the error as ``rebase``. + """ + 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, push, then reset to create a "behind" state + 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"]) + + 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"]) + git_repo.run(["reset", "--hard", "HEAD^"]) + + # cmd.status is called twice: first for get_current_remote_name, then + # for the dirty-tree check. Only the second should return dirty. + real_status = git_repo.cmd.status + + def status_side_effect(**kwargs: t.Any) -> str: + if "untracked_files" in kwargs: + return "M some_file" + return real_status(**kwargs) + + mocker.patch.object( + git_repo.cmd, + "status", + side_effect=status_side_effect, + ) + + mocker.patch.object( + git_repo.cmd, + "rebase", + side_effect=exc.CommandError( + output="CONFLICT (content): Merge conflict in file.txt", + returncode=1, + cmd="git rebase", + ), + ) + + result = git_repo.update_repo() + + assert isinstance(result, SyncResult) + assert result.ok is False + assert len(result.errors) > 0 + assert result.errors[0].step == "rebase" + assert "CONFLICT" in result.errors[0].message + assert result.errors[0].exception is not None + assert isinstance(result.errors[0].exception, exc.CommandError) + + +def test_update_repo_stash_pop_failure_recorded( + create_git_remote_bare_repo: CreateRepoPytestFixtureFn, + tmp_path: pathlib.Path, + mocker: MockerFixture, +) -> None: + """Test that stash pop failure is recorded in SyncResult. + + When ``cmd.stash.pop()`` raises after a successful rebase on a dirty + tree, the error is recorded as ``stash-pop``. + """ + 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, push, then reset to create a "behind" state + 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"]) + + 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"]) + git_repo.run(["reset", "--hard", "HEAD^"]) + + # cmd.status is called twice: first for get_current_remote_name, then + # for the dirty-tree check. Only the second should return dirty. + real_status = git_repo.cmd.status + + def status_side_effect(**kwargs: t.Any) -> str: + if "untracked_files" in kwargs: + return "M some_file" + return real_status(**kwargs) + + mocker.patch.object( + git_repo.cmd, + "status", + side_effect=status_side_effect, + ) + + # stash.pop must fail on both attempts (index=True and without) + mocker.patch.object( + git_repo.cmd.stash, + "pop", + side_effect=exc.CommandError( + output="error: could not restore untracked files", + returncode=1, + cmd="git stash pop", + ), + ) + + result = git_repo.update_repo() + + assert isinstance(result, SyncResult) + assert result.ok is False + assert len(result.errors) > 0 + assert result.errors[0].step == "stash-pop" + assert result.errors[0].exception is not None + assert isinstance(result.errors[0].exception, exc.CommandError) + + +def test_sync_result_multiple_errors() -> None: + """Test that SyncResult can accumulate multiple errors.""" + result = SyncResult() + assert result.ok is True + + result.add_error(step="fetch", message="network error") + assert result.ok is False + assert len(result.errors) == 1 + + result.add_error(step="checkout", message="branch not found") + assert len(result.errors) == 2 + assert result.errors[0].step == "fetch" + assert result.errors[1].step == "checkout" diff --git a/tests/sync/test_hg.py b/tests/sync/test_hg.py index de643f81d..29a41e812 100644 --- a/tests/sync/test_hg.py +++ b/tests/sync/test_hg.py @@ -4,14 +4,20 @@ import pathlib import shutil +import typing as t import pytest from libvcs import exc from libvcs._internal.run import run from libvcs._internal.shortcuts import create_project +from libvcs.pytest_plugin import hg_remote_repo_single_commit_post_init +from libvcs.sync.base import SyncResult from libvcs.sync.hg import HgSync +if t.TYPE_CHECKING: + from libvcs.pytest_plugin import CreateRepoPytestFixtureFn + if not shutil.which("hg"): pytestmark = pytest.mark.skip(reason="hg is not available") @@ -94,9 +100,40 @@ def test_vulnerability_2022_03_12_command_injection( vcs="hg", path="./", ) - with pytest.raises(exc.CommandError): - mercurial_repo.update_repo() + result = mercurial_repo.update_repo() + assert not result.ok, "update_repo() should report failure for malicious URL" + assert any(e.step == "obtain" for e in result.errors), ( + "Error should be recorded under 'obtain' step" + ) assert not pathlib.Path( random_dir / "HELLO", ).exists(), "Prevent command injection in hg aliases" + + +def test_update_repo_pull_failure_returns_sync_result( + projects_path: pathlib.Path, + create_hg_remote_repo: CreateRepoPytestFixtureFn, +) -> None: + """Test that a deleted remote in update_repo() returns SyncResult with error.""" + repo_name = "my_hg_error_project" + hg_remote = create_hg_remote_repo( + remote_repo_post_init=hg_remote_repo_single_commit_post_init, + ) + + hg_repo = HgSync( + url=f"file://{hg_remote}", + path=projects_path / repo_name, + ) + + hg_repo.update_repo() + + shutil.rmtree(hg_remote) + + 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) diff --git a/tests/sync/test_svn.py b/tests/sync/test_svn.py index 0a0b2720b..fe20cb318 100644 --- a/tests/sync/test_svn.py +++ b/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)