From 9ddf157e17134bdafd98ab78949fe3761155c52a Mon Sep 17 00:00:00 2001 From: Eljees <57435526+Eljees@users.noreply.github.com> Date: Sat, 15 Aug 2026 23:46:48 +0300 Subject: [PATCH 1/2] fix: report a missing git instead of an unhandled FileNotFoundError (#303) subprocess.Popen sits outside the try in command_runner.execute, so when the executable itself is missing the FileNotFoundError escapes raw. Both entry points call GitPathTool.set_cwd before their own error handling begins -- diff_quality_tool.main already has `except OSError -> LOGGER.error -> return 1` further down, it simply never gets the chance to run -- so `diff-cover` and `diff-quality` print a traceback when git is not installed. Add ExecutableNotFoundError as a subclass of CommandError, so anything that already catches CommandError keeps working, and raise it from execute() only for the FileNotFoundError that Popen throws. A command that runs and then fails is untouched: tests/test_integration.py requires CommandError to escape main() in that case, and it still does. Both main() functions now wrap set_cwd and report the message through LOGGER.error, returning 1. This is the same class of fix as #380 for #378, which covered the "the linter is not installed" path through run_command_for_code; the "git is not installed" path through execute was left as it was. --- diff_cover/command_runner.py | 33 ++++++- diff_cover/diff_cover_tool.py | 7 +- diff_cover/diff_quality_tool.py | 7 +- .../test_command_runner_missing_executable.py | 91 +++++++++++++++++++ 4 files changed, 135 insertions(+), 3 deletions(-) create mode 100644 tests/test_command_runner_missing_executable.py diff --git a/diff_cover/command_runner.py b/diff_cover/command_runner.py index db49d8e6..32de437d 100644 --- a/diff_cover/command_runner.py +++ b/diff_cover/command_runner.py @@ -1,6 +1,8 @@ import subprocess import sys +GIT_INSTALL_URL = "https://git-scm.com/book/en/v2/Getting-Started-Installing-Git" + class CommandError(Exception): """ @@ -8,6 +10,16 @@ class CommandError(Exception): """ +class ExecutableNotFoundError(CommandError): + """ + Error raised when the executable of a command cannot be found at all. + + This is a `CommandError` so that existing handling keeps working, but a + distinct type so the command line tools can tell "your tooling is not + installed" apart from "the command ran and failed". + """ + + def execute(command, exit_codes=None): """Execute provided command returning the stdout Args: @@ -24,7 +36,12 @@ def execute(command, exit_codes=None): exit_codes = [0] stdout_pipe = subprocess.PIPE - with subprocess.Popen(command, stdout=stdout_pipe, stderr=stdout_pipe) as process: + try: + popen = subprocess.Popen(command, stdout=stdout_pipe, stderr=stdout_pipe) + except FileNotFoundError as exc: + raise ExecutableNotFoundError(_executable_not_found_message(command)) from exc + + with popen as process: try: stdout, stderr = process.communicate() except OSError: @@ -52,6 +69,20 @@ def run_command_for_code(command): return process.returncode +def _executable_not_found_message(command): + """ + Explain that the executable of `command` is not installed or not on PATH. + """ + executable = _ensure_unicode(command[0]) if command else "" + message = ( + f"'{executable}' was not found. diff-cover needs '{executable}' to be " + "installed and on your PATH in order to run." + ) + if executable == "git": + message += f" See {GIT_INSTALL_URL} for installation instructions." + return message + + def _ensure_unicode(text): """ Ensures the text passed in becomes unicode diff --git a/diff_cover/diff_cover_tool.py b/diff_cover/diff_cover_tool.py index 1bf66fc4..ada4e268 100644 --- a/diff_cover/diff_cover_tool.py +++ b/diff_cover/diff_cover_tool.py @@ -7,6 +7,7 @@ import xml.etree.ElementTree as etree from diff_cover import DESCRIPTION, VERSION +from diff_cover.command_runner import ExecutableNotFoundError from diff_cover.config_parser import Tool, get_config from diff_cover.diff_reporter import GitDiffReporter from diff_cover.git_diff import GitDiffFileTool, GitDiffTool @@ -418,7 +419,11 @@ def main(argv=None, directory=None): level = logging.ERROR if quiet else logging.WARNING logging.basicConfig(format="%(message)s", level=level) - GitPathTool.set_cwd(directory) + try: + GitPathTool.set_cwd(directory) + except ExecutableNotFoundError as exc: + LOGGER.error("%s", exc) + return 1 fail_under = arg_dict.get("fail_under") diff_tool = None diff --git a/diff_cover/diff_quality_tool.py b/diff_cover/diff_quality_tool.py index 191b4f6d..45a85e14 100644 --- a/diff_cover/diff_quality_tool.py +++ b/diff_cover/diff_quality_tool.py @@ -14,6 +14,7 @@ import diff_cover from diff_cover import hookspecs +from diff_cover.command_runner import ExecutableNotFoundError from diff_cover.config_parser import Tool, get_config from diff_cover.diff_cover_tool import ( COMPARE_BRANCH_HELP, @@ -339,7 +340,11 @@ def main(argv=None, directory=None): level = logging.ERROR if quiet else logging.WARNING logging.basicConfig(format="%(message)s", level=level) - GitPathTool.set_cwd(directory) + try: + GitPathTool.set_cwd(directory) + except ExecutableNotFoundError as exc: + LOGGER.error("%s", exc) + return 1 fail_under = arg_dict.get("fail_under") tool = arg_dict["violations"] user_options = arg_dict.get("options") diff --git a/tests/test_command_runner_missing_executable.py b/tests/test_command_runner_missing_executable.py new file mode 100644 index 00000000..09b726c1 --- /dev/null +++ b/tests/test_command_runner_missing_executable.py @@ -0,0 +1,91 @@ +# pylint: disable=missing-function-docstring + +"""Tests for the message shown when a required executable is not on PATH. + +See https://github.com/Bachmann1234/diff_cover/issues/303 +""" + +import pytest + +from diff_cover.command_runner import ( + CommandError, + ExecutableNotFoundError, + execute, +) + + +@pytest.fixture +def missing_git(mocker): + """Make every subprocess launch fail the way a missing `git` does.""" + return mocker.patch( + "diff_cover.command_runner.subprocess.Popen", + side_effect=FileNotFoundError(2, "No such file or directory", "git"), + ) + + +@pytest.fixture +def failing_git(mocker): + """Make `git` run and fail, which is a different thing entirely.""" + process = mocker.Mock() + process.returncode = 1 + process.communicate.return_value = (b"", b"fatal: not a git repository") + popen = mocker.patch("diff_cover.command_runner.subprocess.Popen") + popen.return_value.__enter__.return_value = process + popen.return_value.__exit__.return_value = None + return popen + + +def test_execute_turns_a_missing_executable_into_a_helpful_error(missing_git): + with pytest.raises(ExecutableNotFoundError) as exc_info: + execute(["git", "rev-parse", "--show-toplevel"]) + + message = str(exc_info.value) + assert "git" in message + assert "PATH" in message + assert "https://git-scm.com/" in message + + +def test_the_new_error_stays_a_command_error(missing_git): + """Callers that already catch CommandError must keep working.""" + assert issubclass(ExecutableNotFoundError, CommandError) + with pytest.raises(CommandError): + execute(["git", "rev-parse", "--show-toplevel"]) + + +def test_execute_does_not_link_git_docs_for_other_executables(mocker): + mocker.patch( + "diff_cover.command_runner.subprocess.Popen", + side_effect=FileNotFoundError(2, "No such file or directory", "pycodestyle"), + ) + + with pytest.raises(ExecutableNotFoundError) as exc_info: + execute(["pycodestyle", "--version"]) + + message = str(exc_info.value) + assert "pycodestyle" in message + assert "git-scm.com" not in message + + +def test_diff_cover_main_reports_missing_git_without_a_traceback(missing_git, caplog): + from diff_cover.diff_cover_tool import main + + assert main(["diff-cover", "coverage.xml"]) == 1 + assert "PATH" in caplog.text + + +def test_diff_quality_main_reports_missing_git_without_a_traceback(missing_git, caplog): + from diff_cover.diff_quality_tool import main + + assert main(["diff-quality", "--violations", "pycodestyle"]) == 1 + assert "PATH" in caplog.text + + +def test_a_command_that_runs_and_fails_is_left_alone(failing_git): + """Control. Green before and after: only a *missing* binary is reworded.""" + with pytest.raises(CommandError) as exc_info: + execute(["git", "status"]) + + assert not isinstance(exc_info.value, ExecutableNotFoundError) + message = str(exc_info.value) + assert "fatal: not a git repository" in message + assert "PATH" not in message From a555fda0f19e47d050d716ddbfce0cd5dddb8655 Mon Sep 17 00:00:00 2001 From: Eljees <57435526+Eljees@users.noreply.github.com> Date: Sun, 16 Aug 2026 00:08:21 +0300 Subject: [PATCH 2/2] test: assert on the install URL without a substring check CodeQL flagged `assert "https://git-scm.com/" in message` as py/incomplete-url-substring-sanitization. The rule is about URL checks done with substring containment, and a test assertion is written the same way as the pattern it warns about. Import GIT_INSTALL_URL and assert on the whole trailing sentence instead, which is a stricter assertion anyway, and use the constant for the negative case as well. --- tests/test_command_runner_missing_executable.py | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/tests/test_command_runner_missing_executable.py b/tests/test_command_runner_missing_executable.py index 09b726c1..0110fcb6 100644 --- a/tests/test_command_runner_missing_executable.py +++ b/tests/test_command_runner_missing_executable.py @@ -8,6 +8,7 @@ import pytest from diff_cover.command_runner import ( + GIT_INSTALL_URL, CommandError, ExecutableNotFoundError, execute, @@ -42,7 +43,7 @@ def test_execute_turns_a_missing_executable_into_a_helpful_error(missing_git): message = str(exc_info.value) assert "git" in message assert "PATH" in message - assert "https://git-scm.com/" in message + assert message.endswith(f"See {GIT_INSTALL_URL} for installation instructions.") def test_the_new_error_stays_a_command_error(missing_git): @@ -63,7 +64,7 @@ def test_execute_does_not_link_git_docs_for_other_executables(mocker): message = str(exc_info.value) assert "pycodestyle" in message - assert "git-scm.com" not in message + assert GIT_INSTALL_URL not in message def test_diff_cover_main_reports_missing_git_without_a_traceback(missing_git, caplog):