diff --git a/docs/developers-guide.md b/docs/developers-guide.md index 0002b3f3..a8d4e88a 100644 --- a/docs/developers-guide.md +++ b/docs/developers-guide.md @@ -365,8 +365,10 @@ this helper for consistent path handling, including `lading.cli`, `discover_tracked_lockfiles(workspace_root, runner)` filters git-tracked `Cargo.lock` files outside `target/` with adjacent `Cargo.toml` manifests. -Private helpers `_handle_git_ls_files_failure` and `_lockfiles_with_manifests` -perform the error-handling and path-filtering passes respectively. +Private helpers `_raise_git_ls_files_failure` and `_lockfiles_with_manifests` +perform the error-handling and path-filtering passes respectively. Discovery +raises `NotAGitRepositoryError` when the workspace is not under git control; +callers that should skip that condition own the skip policy at their boundary. Lockfile regeneration after `lading bump` is owned by `lading.commands.bump_lockfiles.regenerate_lockfiles`, which runs diff --git a/lading/commands/lockfile.py b/lading/commands/lockfile.py index 2b309018..e5f9db92 100644 --- a/lading/commands/lockfile.py +++ b/lading/commands/lockfile.py @@ -64,6 +64,15 @@ class LockfileDiscoveryError(LadingError): """Raised when git cannot list tracked lockfiles.""" +class NotAGitRepositoryError(LockfileDiscoveryError): + """Raised when lockfile discovery targets a directory outside git control. + + Callers that treat a non-git workspace as "nothing to validate" should + catch this subclass and decide the skip policy themselves; discovery no + longer hides the condition behind a warning and an empty result. + """ + + @dc.dataclass(frozen=True, slots=True) class LockfileFreshness: """Result from validating a lockfile under Cargo's locked mode.""" @@ -73,22 +82,17 @@ class LockfileFreshness: detail: str = "" -def _handle_git_ls_files_failure( +def _raise_git_ls_files_failure( exit_code: int, stdout: str, stderr: str, workspace_root: Path, -) -> tuple[Path, ...] | None: - """Return ``None`` for git success, or an empty result for git failure.""" - if exit_code == 0: - return None +) -> typ.NoReturn: + """Raise the typed discovery error for a failed ``git ls-files`` call.""" detail = command_detail(stdout, stderr) if "not a git repository" in detail.lower(): - LOGGER.warning( - "Skipping Cargo.lock discovery because %s is not a git repository", - workspace_root, - ) - return () + message = f"{workspace_root} is not a git repository" + raise NotAGitRepositoryError(message) # Unlike the other sites, git may exit non-zero with no output at all, so # fall back to the status code before handing the detail to append_detail. fallback = f"git ls-files exited with status {exit_code}" @@ -146,24 +150,30 @@ def discover_tracked_lockfiles( ------- tuple[Path, ...] Git-tracked ``Cargo.lock`` files outside any ``target`` directory and - with an adjacent ``Cargo.toml`` manifest. The helper - :func:`_handle_git_ls_files_failure` handles git failures, and + with an adjacent ``Cargo.toml`` manifest. :func:`_lockfiles_with_manifests` applies manifest and path filtering. + Raises + ------ + NotAGitRepositoryError + If ``workspace_root`` is not under git control. Callers own the skip + policy for that condition. + LockfileDiscoveryError + If ``git ls-files`` fails for any other reason. + Notes ----- - If ``workspace_root`` is not a git repository, discovery logs a warning - through :func:`_handle_git_ls_files_failure` and returns an empty tuple. + Filesystem access is confined to the injected ``manifest_exists`` port; + git access is confined to the injected ``runner``. The function performs + no direct I/O of its own, so integration tests can exercise it against + real directories and unit tests can substitute both ports. """ exit_code, stdout, stderr = runner( ("git", "ls-files", "**/Cargo.lock", "Cargo.lock"), cwd=workspace_root, ) - error_result = _handle_git_ls_files_failure( - exit_code, stdout, stderr, workspace_root - ) - if error_result is not None: - return error_result + if exit_code != 0: + _raise_git_ls_files_failure(exit_code, stdout, stderr, workspace_root) lockfiles = _lockfiles_with_manifests(stdout, workspace_root, manifest_exists) if lockfiles: # Skip a zero-amount increment: it would create a 0-valued counter key diff --git a/lading/commands/publish_preflight.py b/lading/commands/publish_preflight.py index 3ccb862d..17c0d61e 100644 --- a/lading/commands/publish_preflight.py +++ b/lading/commands/publish_preflight.py @@ -33,7 +33,10 @@ import typing as typ from pathlib import Path -from lading.commands.lockfile import CargoLockfileInspectionRepository +from lading.commands.lockfile import ( + CargoLockfileInspectionRepository, + NotAGitRepositoryError, +) from lading.commands.publish_diagnostics import _append_compiletest_diagnostics from lading.commands.publish_errors import PublishPreflightError from lading.commands.publish_execution import _invoke @@ -266,7 +269,16 @@ def _validate_lockfile_freshness( holds a command runner or knows how lockfiles are located and validated (issue #82). """ - tracked = repository.discover_tracked_lockfiles(workspace_root) + try: + tracked = repository.discover_tracked_lockfiles(workspace_root) + except NotAGitRepositoryError: + # Skip policy lives here, not in discovery: a non-git workspace has + # no tracked lockfiles to validate, so pre-flight continues. + LOGGER.warning( + "Skipping lockfile freshness validation: %s is not a git repository", + workspace_root, + ) + return stale_lockfiles = _collect_stale_lockfiles(tracked, repository) if not stale_lockfiles: diff --git a/tests/integration/test_lockfile_discovery.py b/tests/integration/test_lockfile_discovery.py new file mode 100644 index 00000000..0ee33e6f --- /dev/null +++ b/tests/integration/test_lockfile_discovery.py @@ -0,0 +1,102 @@ +"""Integration tests for lockfile discovery against real git repositories. + +Issue #79: these tests exercise :func:`discover_tracked_lockfiles` through +the real subprocess runner and real temporary directories, without stubbing +the internals, so the git and filesystem integrations are covered rather +than mocked layouts. +""" + +from __future__ import annotations + +import shutil +import subprocess +import typing as typ + +import pytest + +from lading.commands import lockfile +from lading.runtime import subprocess_runner + +if typ.TYPE_CHECKING: + from pathlib import Path + +pytestmark = pytest.mark.timeout(60) + + +_GIT_EXECUTABLE = shutil.which("git") or "/usr/bin/git" + + +def _git(repo: Path, *args: str) -> None: + """Run a git command in ``repo`` and fail loudly on error.""" + subprocess.run( # noqa: S603 - fixed argv, test fixture setup + (_GIT_EXECUTABLE, *args), + cwd=repo, + check=True, + capture_output=True, + env={ + "GIT_AUTHOR_NAME": "test", + "GIT_AUTHOR_EMAIL": "test@example.com", + "GIT_COMMITTER_NAME": "test", + "GIT_COMMITTER_EMAIL": "test@example.com", + "PATH": "/usr/bin:/bin", + "HOME": str(repo), + }, + ) + + +def _init_repo(repo: Path) -> None: + """Initialise a git repository with deterministic identity settings.""" + repo.mkdir(parents=True, exist_ok=True) + _git(repo, "init", "--quiet") + + +def _add_crate(repo: Path, relative: str, *, with_manifest: bool = True) -> Path: + """Create a crate directory with a Cargo.lock (and optional manifest).""" + crate_dir = repo / relative if relative else repo + crate_dir.mkdir(parents=True, exist_ok=True) + lock = crate_dir / "Cargo.lock" + lock.write_text("# lock\n", encoding="utf-8") + if with_manifest: + (crate_dir / "Cargo.toml").write_text( + '[package]\nname = "x"\nversion = "0.1.0"\n', encoding="utf-8" + ) + return lock + + +def test_discovery_returns_tracked_lockfiles_with_manifests(tmp_path: Path) -> None: + """Tracked lockfiles with adjacent manifests are discovered for real.""" + repo = tmp_path / "repo" + _init_repo(repo) + root_lock = _add_crate(repo, "") + nested_lock = _add_crate(repo, "crates/alpha") + _add_crate(repo, "no-manifest", with_manifest=False) + target_lock = _add_crate(repo, "target/debug") + _git(repo, "add", "-A") + + result = lockfile.discover_tracked_lockfiles(repo, subprocess_runner) + + assert set(result) == {root_lock, nested_lock} + assert target_lock not in result + + +def test_discovery_ignores_untracked_lockfiles(tmp_path: Path) -> None: + """Lockfiles not in the git index are not discovered.""" + repo = tmp_path / "repo" + _init_repo(repo) + tracked = _add_crate(repo, "tracked") + _git(repo, "add", "tracked") + untracked = _add_crate(repo, "untracked") + + result = lockfile.discover_tracked_lockfiles(repo, subprocess_runner) + + assert tracked in result + assert untracked not in result + + +def test_discovery_raises_for_non_git_directory(tmp_path: Path) -> None: + """A plain directory raises the typed not-a-repository error.""" + workspace = tmp_path / "plain" + _add_crate(workspace, "") + + with pytest.raises(lockfile.NotAGitRepositoryError): + lockfile.discover_tracked_lockfiles(workspace, subprocess_runner) diff --git a/tests/unit/publish/test_preflight_lockfile_validation.py b/tests/unit/publish/test_preflight_lockfile_validation.py index 1c84b00d..48f69569 100644 --- a/tests/unit/publish/test_preflight_lockfile_validation.py +++ b/tests/unit/publish/test_preflight_lockfile_validation.py @@ -10,6 +10,7 @@ from __future__ import annotations import dataclasses as dc +import logging import typing as typ from pathlib import Path @@ -176,6 +177,50 @@ def test_validate_lockfile_freshness_surfaces_cargo_failures(tmp_path: Path) -> publish_preflight._validate_lockfile_freshness(tmp_path, repository=repository) +def test_validate_lockfile_freshness_skips_non_git_workspaces( + tmp_path: Path, + caplog: pytest.LogCaptureFixture, +) -> None: + """The skip policy for non-git workspaces lives at the pre-flight caller. + + Issue #79: discovery raises ``NotAGitRepositoryError`` instead of hiding + the condition; pre-flight catches it at the port boundary, warns, and + continues without probing any lockfile. + """ + caplog.set_level(logging.WARNING, logger="lading.commands.publish_preflight") + + @dc.dataclass + class _NonGitRepository: + """Recording repository double that reports a non-git workspace.""" + + discovered_roots: list[Path] = dc.field(default_factory=list) + + def discover_tracked_lockfiles(self, workspace_root: Path) -> tuple[Path, ...]: + """Record the call and raise the typed non-git discovery error.""" + self.discovered_roots.append(workspace_root) + message = f"{workspace_root} is not a git repository" + raise lockfile.NotAGitRepositoryError(message) + + def validate_lockfile_freshness( + self, manifest_path: Path + ) -> lockfile.LockfileFreshness: + """Fail: no lockfile should be validated for a non-git workspace.""" + pytest.fail( + f"no lockfile should be validated for a non-git workspace: " + f"{manifest_path}" + ) + + repository = _NonGitRepository() + + publish_preflight._validate_lockfile_freshness(tmp_path, repository=repository) + + assert repository.discovered_roots == [tmp_path] + assert any( + "Skipping lockfile freshness validation" in message + for message in caplog.messages + ) + + def test_validate_lockfile_freshness_classifies_every_lockfile( tmp_path: Path, ) -> None: diff --git a/tests/unit/test_lockfile.py b/tests/unit/test_lockfile.py index a52c0ef7..083f794b 100644 --- a/tests/unit/test_lockfile.py +++ b/tests/unit/test_lockfile.py @@ -183,10 +183,10 @@ def manifest_exists(manifest_path: Path) -> bool: assert probed == [tmp_path / "Cargo.toml", tmp_path / "nested" / "Cargo.toml"] -def test_discover_tracked_lockfiles_handles_non_git_directory( +def test_discover_tracked_lockfiles_raises_for_non_git_directory( tmp_path: Path, ) -> None: - """Non-git workspaces do not abort lockfile discovery.""" + """Non-git workspaces surface a typed error instead of a silent skip.""" (tmp_path / "Cargo.lock").write_text("", encoding="utf-8") def runner( @@ -197,11 +197,10 @@ def runner( ) -> tuple[int, str, str]: return 128, "", "fatal: not a git repository" - result = lockfile.discover_tracked_lockfiles(tmp_path, runner) - assert result == (), ( - "discovery should not abort on non-git errors; " - f"expected empty tuple, got {result!r}" - ) + with pytest.raises( + lockfile.NotAGitRepositoryError, match="is not a git repository" + ): + lockfile.discover_tracked_lockfiles(tmp_path, runner) def test_discover_tracked_lockfiles_raises_on_git_failure(tmp_path: Path) -> None: