From 5db5c0c3a5c25faaf52bf636bc3546ddf3162e03 Mon Sep 17 00:00:00 2001 From: suna Date: Fri, 14 Aug 2026 13:58:32 +0200 Subject: [PATCH] fix: detect if git is installed and exit with a clear error --- .flake8 | 4 + git_preflight.py | 77 ++++++++++++++ plain_modules.py | 13 ++- tests/test_git_preflight.py | 198 ++++++++++++++++++++++++++++++++++++ 4 files changed, 287 insertions(+), 5 deletions(-) create mode 100644 git_preflight.py create mode 100644 tests/test_git_preflight.py diff --git a/.flake8 b/.flake8 index 26c5caa6..8953debd 100644 --- a/.flake8 +++ b/.flake8 @@ -20,6 +20,10 @@ exclude = per-file-ignores = __init__.py: F401 tests/*: U100 +# plain_modules must call git_preflight.require_git() before importing GitPython, +# which raises from module scope when git is missing or broken. That call is a +# statement between imports, so every import below it trips E402. + plain_modules.py: E402 max-complexity = 15 enable-extensions = U100 # Plugin-specific configuration diff --git a/git_preflight.py b/git_preflight.py new file mode 100644 index 00000000..f380595b --- /dev/null +++ b/git_preflight.py @@ -0,0 +1,77 @@ +"""Verify a working git is available before GitPython is imported. + +GitPython probes for the git executable when it is first imported and raises from +module scope if it is missing or broken. That happens while plain2code is still +importing its own modules, so a check inside main() cannot intercept it -- the +user gets a raw traceback instead of an explanation. + +Two failure modes need separate handling. A missing git is simply absent from +PATH. A broken git resolves but cannot run: on macOS without the Command Line +Tools /usr/bin/git is a stub that exits non-zero, and stale shims behave the same +way. GIT_PYTHON_REFRESH=quiet does not help with the latter, because GitPython +only consults that setting for a git it cannot find. A PATH lookup alone is +therefore not sufficient, so this module runs ``git version`` -- mirroring the +``git_available`` check in install/bash/install.sh. +""" + +from __future__ import annotations + +import shutil +import subprocess +import sys + +from plain2code_console import console + +# Long enough for a cold-cache process spawn, short enough that a wedged shim +# cannot hang the CLI indefinitely. +GIT_VERSION_TIMEOUT_SECONDS = 10 + +GIT_MISSING_MESSAGE = "git is not installed. Please install git and try again." +GIT_BROKEN_MESSAGE = "git is installed but not working. Please repair your git installation and try again." +MACOS_BROKEN_GIT_HINT = ( + "This usually means the Command Line Tools are missing; install them with 'xcode-select --install'." +) + + +def broken_git_message() -> str: + """The broken-git message, with a platform hint where there is a likely cause.""" + if sys.platform == "darwin": + return f"{GIT_BROKEN_MESSAGE}\n{MACOS_BROKEN_GIT_HINT}" + + return GIT_BROKEN_MESSAGE + + +def find_git() -> str | None: + """Return the path to the git executable, or None if it is not on PATH.""" + return shutil.which("git") + + +def git_runs(git_path: str) -> bool: + """Whether ``git version`` actually succeeds for the given executable. + + A resolvable name can still be unusable, so the command is run rather than + assumed. Any failure to execute it at all counts as unusable. + """ + try: + result = subprocess.run( + [git_path, "version"], + capture_output=True, + timeout=GIT_VERSION_TIMEOUT_SECONDS, + ) + except (OSError, subprocess.SubprocessError): + return False + + return result.returncode == 0 + + +def require_git() -> None: + """Exit with a clean message unless a working git is available.""" + git_path = find_git() + + if git_path is None: + console.error(f"{GIT_MISSING_MESSAGE}\n") + sys.exit(1) + + if not git_runs(git_path): + console.error(f"{broken_git_message()}\n") + sys.exit(1) diff --git a/plain_modules.py b/plain_modules.py index cda701ca..482a5fc2 100644 --- a/plain_modules.py +++ b/plain_modules.py @@ -4,12 +4,15 @@ import shutil from functools import cached_property -from plain2code_exceptions import GitNotInstalledError, MissingPreviousFunctionalitiesError, ModuleDoesNotExistError +# GitPython probes for the git executable when it is first imported and reports a +# missing or broken one by raising from module scope, which would crash the CLI +# with a traceback before main() could explain it. Diagnose git ourselves first. +from git_preflight import require_git +from plain2code_exceptions import MissingPreviousFunctionalitiesError, ModuleDoesNotExistError -try: - from git.exc import NoSuchPathError -except ImportError: - raise GitNotInstalledError("git is not installed. Please install git and try again.") +require_git() + +from git.exc import NoSuchPathError import git_utils import metadata_utils diff --git a/tests/test_git_preflight.py b/tests/test_git_preflight.py new file mode 100644 index 00000000..bcc00173 --- /dev/null +++ b/tests/test_git_preflight.py @@ -0,0 +1,198 @@ +"""Regression tests for issue #133. + +When git is unusable the CLI must report a clean, one-line diagnosis and exit +non-zero -- never dump a Python traceback. + +The failure is import-time: GitPython probes for the git executable when it is +first imported and raises from module scope, before ``main()`` runs. A check +inside ``main()`` cannot intercept it, so these tests run the CLI in a +subprocess and assert on what a user would actually see. Two failure modes are +covered, because they surface through different GitPython paths: + +- git missing entirely -- nothing named ``git`` on PATH. +- git present but broken -- a ``git`` that resolves but exits non-zero, as on + macOS without the Command Line Tools. ``GIT_PYTHON_REFRESH=quiet`` does not + suppress this one, which is why a PATH lookup alone is not a sufficient check. + +``sys.executable`` is launched by absolute path, so replacing PATH does not stop +Python from starting -- it only controls which git, if any, can be found. +""" + +import os +import subprocess +import sys +from pathlib import Path + +import pytest + +from git_preflight import ( + GIT_BROKEN_MESSAGE, + GIT_MISSING_MESSAGE, + MACOS_BROKEN_GIT_HINT, + broken_git_message, + git_runs, +) + +REPO_ROOT = Path(__file__).resolve().parents[1] + +TRACEBACK_MARKER = "Traceback (most recent call last)" + +# Every entry point, including the ones that do no git work themselves: the crash +# was in the import chain, so it hit them all equally. +CLI_INVOCATIONS = [ + pytest.param(["--version"], id="version"), + pytest.param(["--status"], id="status"), + pytest.param(["does-not-exist.plain"], id="render"), +] + + +def _env(path_dir=None): + """The current environment, with PATH optionally replaced. + + GitPython overrides are dropped so the PATH search is the only thing + deciding whether git is found. + """ + env = os.environ.copy() + env.pop("GIT_PYTHON_GIT_EXECUTABLE", None) + env.pop("GIT_PYTHON_REFRESH", None) + if path_dir is not None: + env["PATH"] = str(path_dir) + env.setdefault("CODEPLAIN_API_KEY", "dummy") + return env + + +def _run_cli(args, env): + result = subprocess.run( + [sys.executable, "plain2code.py", *args], + cwd=REPO_ROOT, + env=env, + capture_output=True, + text=True, + timeout=120, + ) + return result.returncode, result.stdout + result.stderr + + +def _normalize(output): + """Collapse whitespace so assertions survive console line wrapping.""" + return " ".join(output.split()) + + +@pytest.fixture +def no_git_dir(tmp_path): + """A directory containing no git at all, used as the whole PATH.""" + d = tmp_path / "no-git" + d.mkdir() + return d + + +@pytest.fixture +def broken_git_dir(tmp_path): + """A directory whose git resolves but always fails, used as the whole PATH. + + This is the shape of macOS without the Command Line Tools: the name is + there, running it is not possible. + """ + d = tmp_path / "broken-git" + d.mkdir() + if sys.platform == "win32": + git = d / "git.bat" + git.write_text("@echo off\nexit /b 127\n") + else: + git = d / "git" + git.write_text("#!/bin/sh\nexit 127\n") + git.chmod(0o755) + return d + + +@pytest.mark.parametrize("args", CLI_INVOCATIONS) +def test_missing_git_reports_cleanly(args, no_git_dir): + rc, output = _run_cli(args, _env(no_git_dir)) + + assert GIT_MISSING_MESSAGE in _normalize(output), output + assert TRACEBACK_MARKER not in output, output + assert rc == 1, output + + +@pytest.mark.parametrize("args", CLI_INVOCATIONS) +def test_broken_git_reports_cleanly(args, broken_git_dir): + # A git that resolves but cannot run must be diagnosed as broken rather than + # missing -- and must not reach GitPython's import-time traceback. + rc, output = _run_cli(args, _env(broken_git_dir)) + normalized = _normalize(output) + + assert GIT_BROKEN_MESSAGE in normalized, output + assert GIT_MISSING_MESSAGE not in normalized, output + assert TRACEBACK_MARKER not in output, output + assert rc == 1, output + + +@pytest.mark.parametrize("args", CLI_INVOCATIONS) +def test_missing_git_does_not_leak_gitpython_advice(args, no_git_dir): + # GitPython's own message coaches the user about $GIT_PYTHON_REFRESH and + # git.refresh(), which are irrelevant to someone who just needs to install + # git. None of it should reach the user. + _, output = _run_cli(args, _env(no_git_dir)) + + assert "GIT_PYTHON_REFRESH" not in output, output + assert "Bad git executable" not in output, output + + +# plain_modules is deliberately not tested as a standalone import: it has a +# pre-existing circular import with file_utils that fails whether or not git is +# present, so the assertion would hold for the wrong reason. +@pytest.mark.parametrize("module", ["plain2code", "file_utils"]) +@pytest.mark.parametrize("path_fixture", ["no_git_dir", "broken_git_dir"]) +def test_importing_git_dependent_module_exits_cleanly(module, path_fixture, request): + """Importing a module in the CLI's git-dependent import chain must diagnose + git rather than traceback.""" + path_dir = request.getfixturevalue(path_fixture) + result = subprocess.run( + [sys.executable, "-c", f"import {module}"], + cwd=REPO_ROOT, + env=_env(path_dir), + capture_output=True, + text=True, + timeout=120, + ) + output = result.stdout + result.stderr + + assert TRACEBACK_MARKER not in output, output + assert result.returncode == 1, output + + +def test_cli_works_when_git_is_present(): + # The check must not get in the way of normal use. + rc, output = _run_cli(["--version"], _env()) + + assert rc == 0, output + assert "codeplain version" in output, output + + +def test_git_runs_accepts_real_git(): + import shutil + + git_path = shutil.which("git") + assert git_path is not None, "test environment has no git" + assert git_runs(git_path) is True + + +def test_git_runs_rejects_failing_git(broken_git_dir): + broken = broken_git_dir / ("git.bat" if sys.platform == "win32" else "git") + + assert git_runs(str(broken)) is False + + +def test_git_runs_rejects_nonexistent_path(tmp_path): + # OSError from exec must be treated as unusable, not propagated. + assert git_runs(str(tmp_path / "definitely-not-here")) is False + + +@pytest.mark.skipif(sys.platform != "darwin", reason="hint is macOS-specific") +def test_broken_git_message_hints_at_command_line_tools_on_macos(): + assert MACOS_BROKEN_GIT_HINT in broken_git_message() + + +@pytest.mark.skipif(sys.platform == "darwin", reason="hint is macOS-specific") +def test_broken_git_message_has_no_macos_hint_elsewhere(): + assert broken_git_message() == GIT_BROKEN_MESSAGE