From 7032e751e3b09d6018e9954888f9af578fab9326 Mon Sep 17 00:00:00 2001 From: Xianpeng Shen <3353385+shenxianpeng@users.noreply.github.com> Date: Tue, 10 Nov 2026 07:56:50 +0000 Subject: [PATCH] feat: add --lines-changed-only to the clang-format hook and test with prek clang-format formats whole files by default. With --lines-changed-only the hook passes the lines that the commit or push changes to clang-format's --lines, one run per file, and falls back to whole files when the changed lines are unknown. The option parser no longer accepts abbreviations, so clang-format's own --lines is not read as a short form of the new option. The integration script takes RUNNER=prek, and CI runs it once with prek. --- .github/workflows/test.yml | 5 ++ README.md | 24 +++++- cpp_linter_hooks/clang_format.py | 84 ++++++++++++++------ cpp_linter_hooks/clang_tidy.py | 60 ++------------ cpp_linter_hooks/util.py | 54 +++++++++++++ testing/run.sh | 9 ++- tests/test_clang_format.py | 130 +++++++++++++++++++++++++++++++ tests/test_clang_tidy.py | 10 +-- 8 files changed, 288 insertions(+), 88 deletions(-) diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index 38d309b..5348e58 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -52,3 +52,8 @@ jobs: verbose: true # optional (default = false) - name: Test cpp-linter-hooks run: bash testing/run.sh + - name: Test cpp-linter-hooks with prek + if: matrix.os == 'ubuntu-latest' && matrix.python-version == '3.13' + run: bash testing/run.sh + env: + RUNNER: prek diff --git a/README.md b/README.md index 897c71b..873b058 100644 --- a/README.md +++ b/README.md @@ -6,7 +6,8 @@ [![part of cpp-linter](https://img.shields.io/badge/part%20of-cpp--linter-ffc20a?labelColor=454a63)](https://cpp-linter.github.io/) [pre-commit](https://pre-commit.com/) hooks that pip-install the clang-format and clang-tidy -version you pin, on every developer's machine. +version you pin, on every developer's machine. They also run with +[prek](https://prek.j178.dev/), which the tests cover too. [Website](https://cpp-linter.github.io/) · [Get started](https://cpp-linter.github.io/getting-started/#before-every-commit) · @@ -43,7 +44,7 @@ This uses the newest clang-format and clang-tidy. To choose a version, add `--ve ### Clang tool version -The `rev` tag (e.g. `v1.8.1`) is the version of these hooks, not of clang-format or clang-tidy. +The `rev` tag is the version of these hooks, not of clang-format or clang-tidy. Choose the clang tool version with `--version`: ```yaml @@ -142,6 +143,24 @@ int main() {for (;;) break; printf("Hello world!\n");return 0;} ^ ``` +#### Format only changed lines + +By default the hook formats whole files. In a code base that was never formatted, that turns a +one-line change into a large diff. Add `--lines-changed-only` to format only the lines that the +commit adds or changes: + +```yaml + - id: clang-format + args: [--style=file, --lines-changed-only] +``` + +The hook reads the changes with `git diff`, in the same way as the clang-tidy hook (see +[Only changed lines](https://github.com/cpp-linter/cpp-linter-hooks#only-changed-lines)), and +passes them to clang-format's `--lines`. clang-format can still touch a nearby line, because it +formats whole statements. The hook formats every line when it cannot tell which lines changed, +for example with `pre-commit run --all-files` on a clean working tree, and when `args` already +has `--lines`. + ### clang-tidy clang-tidy 22 and newer enable no checks by themselves. Without a `.clang-tidy` file or @@ -369,6 +388,7 @@ mirror of the clang-format wheel. | Supports passing format style string | via `--style` | via `--style` | | Verbose output | via `--verbose` | via `--verbose` | | Dry-run mode | via `--dry-run` | via `--dry-run --Werror` | +| Format only changed lines | via `--lines-changed-only` | No | | Auto-fix mode | via `--fix` (clang-tidy only) | No | | Compilation database support | auto-detect or `--compile-commands` | No | diff --git a/cpp_linter_hooks/clang_format.py b/cpp_linter_hooks/clang_format.py index db7f888..248f8c9 100644 --- a/cpp_linter_hooks/clang_format.py +++ b/cpp_linter_hooks/clang_format.py @@ -4,9 +4,16 @@ import sys from argparse import ArgumentParser -from cpp_linter_hooks.util import file_argument, resolve_install_with_diagnostics +from cpp_linter_hooks.changed_lines import changed_lines +from cpp_linter_hooks.util import ( + file_argument, + resolve_install_with_diagnostics, + split_source_files, +) -parser = ArgumentParser() +# No abbreviations: clang-format's own --lines must not be read as a short +# form of --lines-changed-only. +parser = ArgumentParser(allow_abbrev=False) parser.add_argument( "--version", default=None, @@ -18,6 +25,35 @@ parser.add_argument( "-v", "--verbose", action="store_true", help="Enable verbose output" ) +parser.add_argument( + "--lines-changed-only", + action="store_true", + help=( + "Format only the lines that the commit or push changes; every line " + "is formatted when the changed lines are unknown, for example with " + "--all-files" + ), +) + + +def _changed_lines_commands(command: list[str], args: list[str]) -> list[list[str]]: + """Return one clang-format command per file, limited to its changed lines. + + clang-format takes ``--lines`` for one file at a time. A file with no + added lines needs no run. When the changed lines are unknown, or the + arguments already name lines, the one command for all files is returned. + """ + options, files = split_source_files(args) + if any(a.lstrip("-").split("=")[0] == "lines" for a in options): + return [[*command, *args]] + lines = changed_lines(files) + if lines is None: + return [[*command, *args]] + return [ + [*command, *options, *(f"--lines={a}:{b}" for a, b in lines[f]), f] + for f in files + if lines[f] + ] def run_clang_format(args=None) -> tuple[int, str]: @@ -43,32 +79,32 @@ def run_clang_format(args=None) -> tuple[int, str]: ): command.append("--Werror") - command.extend(other_args) + commands = ( + _changed_lines_commands(command, other_args) + if hook_args.lines_changed_only + else [[*command, *other_args]] + ) + retval, output = 0, "" try: - # Run the clang-format command with captured output - sp = subprocess.run( - command, - capture_output=True, - encoding="utf-8", - # clang-format prints source lines, which may not be UTF-8. - errors="replace", - check=False, - ) - - # Combine stdout and stderr for complete output - output = (sp.stdout or "") + (sp.stderr or "") - - retval = sp.returncode - - # Print verbose information if requested - if hook_args.verbose: - _print_verbose_info(command, retval, output) - - return retval, output - + for command in commands: + sp = subprocess.run( + command, + capture_output=True, + encoding="utf-8", + # clang-format prints source lines, which may not be UTF-8. + errors="replace", + check=False, + ) + # Combine stdout and stderr for complete output + command_output = (sp.stdout or "") + (sp.stderr or "") + if hook_args.verbose: + _print_verbose_info(command, sp.returncode, command_output) + retval = retval or sp.returncode + output += command_output except FileNotFoundError as e: return 1, str(e) + return retval, output def _print_verbose_info(command: list, retval: int, output: str) -> None: diff --git a/cpp_linter_hooks/clang_tidy.py b/cpp_linter_hooks/clang_tidy.py index 96e0851..0c0f7a8 100644 --- a/cpp_linter_hooks/clang_tidy.py +++ b/cpp_linter_hooks/clang_tidy.py @@ -15,7 +15,11 @@ diff_range, normalize, ) -from cpp_linter_hooks.util import file_argument, resolve_install_with_diagnostics +from cpp_linter_hooks.util import ( + file_argument, + resolve_install_with_diagnostics, + split_source_files, +) COMPILE_DB_SEARCH_DIRS = ["build", "out", "cmake-build-debug", "_build", "builddir"] # Searched after COMPILE_DB_SEARCH_DIRS. When a pattern matches several @@ -27,33 +31,6 @@ # Windows limits a whole command line to 32,767 characters and Linux limits one # argument to 128 KiB. Commands that carry a --line-filter stay below this. COMMAND_LINE_BUDGET = 30_000 if os.name == "nt" else 100_000 -SOURCE_FILE_SUFFIXES = { - ".c", - ".cc", - ".ccm", - ".cp", - ".cpp", - ".cppm", - ".cxx", - ".cxxm", - ".c++", - ".c++m", - ".cu", - ".cuh", - ".h", - ".hh", - ".hpp", - ".hxx", - ".h++", - ".ino", - ".ipp", - ".inl", - ".ixx", - ".m", - ".mm", - ".tpp", - ".txx", -} COMPILE_COMMANDS_HINT = """\ Generate compile_commands.json with one of: CMake: cmake -S . -B build -DCMAKE_EXPORT_COMPILE_COMMANDS=ON @@ -590,31 +567,6 @@ def _append_notes(output: str, notes: list[str]) -> str: return output.rstrip("\n") + separator + "\n".join(notes) -def _looks_like_source_file(path: str) -> bool: - """Return whether a path has a recognized C or C++ source suffix.""" - return Path(path).suffix.lower() in SOURCE_FILE_SUFFIXES - - -def _split_source_files(args: list[str]) -> tuple[list[str], list[str]]: - """Split clang-tidy options from the source files pre-commit appends. - - Going back from the end, an argument is a source file when it has a C or - C++ suffix, or when it is an existing file that does not follow an option, - which could take it as its value (``--export-fixes fixes.yaml``). The - second case covers suffixes added with ``types_or`` or ``files``. - """ - split_idx = len(args) - while split_idx > 0: - arg = args[split_idx - 1] - previous = args[split_idx - 2] if split_idx > 1 else "" - if not _looks_like_source_file(arg) and ( - previous.startswith("-") or not os.path.isfile(arg) - ): - break - split_idx -= 1 - return args[:split_idx], args[split_idx:] - - def _combine_outputs(results: list[tuple[int, str]]) -> str: """Join non-empty clang-tidy outputs from multiple executions.""" return "\n".join(output.rstrip("\n") for _, output in results if output) @@ -685,7 +637,7 @@ def run_clang_tidy(args=None) -> tuple[int, str]: if error is not None: return error - clang_tidy_args, source_files = _split_source_files(other_args) + clang_tidy_args, source_files = split_source_files(other_args) source_files = [file_argument(f) for f in source_files] clang_tidy_args, notes = _repair_check_args(clang_tidy_args) # clang-tidy reads everything after "--" as compiler arguments, so the diff --git a/cpp_linter_hooks/util.py b/cpp_linter_hooks/util.py index 5919eee..d25b244 100644 --- a/cpp_linter_hooks/util.py +++ b/cpp_linter_hooks/util.py @@ -14,6 +14,35 @@ LOG = logging.getLogger(__name__) +SOURCE_FILE_SUFFIXES = { + ".c", + ".cc", + ".ccm", + ".cp", + ".cpp", + ".cppm", + ".cxx", + ".cxxm", + ".c++", + ".c++m", + ".cu", + ".cuh", + ".h", + ".hh", + ".hpp", + ".hxx", + ".h++", + ".ino", + ".ipp", + ".inl", + ".ixx", + ".m", + ".mm", + ".tpp", + ".txx", +} + + def file_argument(arg: str) -> str: """Return a file name that a clang tool cannot read as an option. @@ -27,6 +56,31 @@ def file_argument(arg: str) -> str: return arg +def _looks_like_source_file(path: str) -> bool: + """Return whether a path has a recognized C or C++ source suffix.""" + return Path(path).suffix.lower() in SOURCE_FILE_SUFFIXES + + +def split_source_files(args: list[str]) -> tuple[list[str], list[str]]: + """Split a clang tool's options from the source files pre-commit appends. + + Going back from the end, an argument is a source file when it has a C or + C++ suffix, or when it is an existing file that does not follow an option, + which could take it as its value (``--export-fixes fixes.yaml``). The + second case covers suffixes added with ``types_or`` or ``files``. + """ + split_idx = len(args) + while split_idx > 0: + arg = args[split_idx - 1] + previous = args[split_idx - 2] if split_idx > 1 else "" + if not _looks_like_source_file(arg) and ( + previous.startswith("-") or not os.path.isfile(arg) + ): + break + split_idx -= 1 + return args[:split_idx], args[split_idx:] + + @lru_cache(maxsize=1) def _env_paths() -> dict[str, str]: """Return this Python environment's install paths, which never change at runtime.""" diff --git a/testing/run.sh b/testing/run.sh index ba27160..82ca45d 100755 --- a/testing/run.sh +++ b/testing/run.sh @@ -11,6 +11,9 @@ set -euo pipefail +# The tool that runs the hooks: pre-commit, or prek with RUNNER=prek. +RUNNER="${RUNNER:-pre-commit}" + PASS=0 FAIL=0 @@ -27,12 +30,12 @@ run_test() { local files="$3" local expect_nonzero="$4" - echo "---- $description ----" - uvx pre-commit clean + echo "---- $description ($RUNNER) ----" + uvx "$RUNNER" clean local output # shellcheck disable=SC2086 # word-splitting of $files is intentional - if output=$(uvx pre-commit run -c "$config" --files $files 2>&1); then + if output=$(uvx "$RUNNER" run -c "$config" --files $files 2>&1); then local got_nonzero=false else local got_nonzero=true diff --git a/tests/test_clang_format.py b/tests/test_clang_format.py index d92590c..35ec7a2 100644 --- a/tests/test_clang_format.py +++ b/tests/test_clang_format.py @@ -5,6 +5,7 @@ import pytest +from cpp_linter_hooks import clang_format from cpp_linter_hooks.clang_format import main, parser, run_clang_format @@ -361,3 +362,132 @@ def test_run_clang_format_uses_the_resolved_executable(mock_clang_format): run_clang_format(["a.cpp"]) assert mock_run.call_args.args[0] == [str(tool), "-i", "a.cpp"] + + +# --- --lines-changed-only --- + + +def test_lines_changed_only_runs_once_per_file_with_its_lines( + mock_clang_format, monkeypatch +): + _, mock_run = mock_clang_format + monkeypatch.setattr( + clang_format, + "changed_lines", + lambda files: {"a.cpp": [[3, 3], [7, 8]], "b.cpp": [[1, 2]], "c.cpp": []}, + ) + + assert run_clang_format( + ["--lines-changed-only", "--style=Google", "a.cpp", "b.cpp", "c.cpp"] + ) == (0, "") + + # c.cpp has no added lines, so it is not formatted. + assert [call.args[0] for call in mock_run.call_args_list] == [ + ["clang-format", "-i", "--style=Google", "--lines=3:3", "--lines=7:8", "a.cpp"], + ["clang-format", "-i", "--style=Google", "--lines=1:2", "b.cpp"], + ] + + +def test_lines_changed_only_formats_every_line_when_the_lines_are_unknown( + mock_clang_format, monkeypatch +): + _, mock_run = mock_clang_format + monkeypatch.setattr(clang_format, "changed_lines", lambda files: None) + + run_clang_format(["--lines-changed-only", "--style=Google", "a.cpp", "b.cpp"]) + + mock_run.assert_called_once() + assert mock_run.call_args.args[0] == [ + "clang-format", + "-i", + "--style=Google", + "a.cpp", + "b.cpp", + ] + + +@pytest.mark.parametrize("lines", (["--lines=1:2"], ["-lines=1:2"], ["--lines", "1:2"])) +def test_lines_changed_only_keeps_lines_given_in_args( + mock_clang_format, monkeypatch, lines +): + _, mock_run = mock_clang_format + monkeypatch.setattr( + clang_format, "changed_lines", lambda files: {"a.cpp": [[5, 5]]} + ) + + run_clang_format(["--lines-changed-only", *lines, "a.cpp"]) + + mock_run.assert_called_once() + assert mock_run.call_args.args[0] == ["clang-format", "-i", *lines, "a.cpp"] + + +def test_lines_changed_only_runs_nothing_without_added_lines( + mock_clang_format, monkeypatch +): + _, mock_run = mock_clang_format + monkeypatch.setattr(clang_format, "changed_lines", lambda files: {"a.cpp": []}) + + assert run_clang_format(["--lines-changed-only", "a.cpp"]) == (0, "") + + mock_run.assert_not_called() + + +def test_lines_changed_only_reports_every_failing_file(mock_clang_format, monkeypatch): + _, mock_run = mock_clang_format + monkeypatch.setattr( + clang_format, + "changed_lines", + lambda files: {name: [[1, 1]] for name in files}, + ) + mock_run.side_effect = [ + subprocess.CompletedProcess(args=[], returncode=0, stdout="", stderr=""), + subprocess.CompletedProcess(args=[], returncode=1, stdout="", stderr="b\n"), + subprocess.CompletedProcess(args=[], returncode=2, stdout="", stderr="c\n"), + ] + + assert run_clang_format( + ["--lines-changed-only", "--dry-run", "a.cpp", "b.cpp", "c.cpp"] + ) == (1, "b\nc\n") + + assert mock_run.call_args.args[0] == [ + "clang-format", + "-i", + "--Werror", + "--dry-run", + "--lines=1:1", + "c.cpp", + ] + + +def test_lines_changed_only_formats_only_the_staged_lines(tmp_path, monkeypatch): + """The real clang-format leaves the unformatted lines of older commits alone.""" + + def git(*args): + subprocess.run( + [ + "git", + "-c", + "user.name=Test", + "-c", + "user.email=test@example.com", + "-c", + "commit.gpgsign=false", + *args, + ], + cwd=tmp_path, + check=True, + capture_output=True, + ) + + source = tmp_path / "a.c" + source.write_text("int old = 1;\n\nint other = 2;\n") + git("init", "-q") + git("add", "a.c") + git("commit", "-q", "-m", "init") + source.write_text("int old = 1;\n\nint added = 3;\n") + git("add", "a.c") + monkeypatch.chdir(tmp_path) + + assert run_clang_format(["--lines-changed-only", "--style=LLVM", "a.c"]) == (0, "") + + assert source.read_text() == "int old = 1;\n\nint added = 3;\n" diff --git a/tests/test_clang_tidy.py b/tests/test_clang_tidy.py index 2b52736..3a7ce2f 100644 --- a/tests/test_clang_tidy.py +++ b/tests/test_clang_tidy.py @@ -26,12 +26,12 @@ _positive_int, _repair_check_args, _set_macos_sdkroot, - _split_source_files, _strip_noise, main, parser, run_clang_tidy, ) +from cpp_linter_hooks.util import split_source_files def test_all_arguments_have_help(): @@ -708,7 +708,7 @@ def test_append_guidance_appends_every_matching_hint(): ), ) def test_split_source_files(args, expected): - assert _split_source_files(args) == expected + assert split_source_files(args) == expected def test_combine_outputs_skips_empty_results(): @@ -1554,7 +1554,7 @@ def test_fixed_directories_win_over_patterns(tmp_path, monkeypatch): "name", ("a.mm", "a.cppm", "a.ccm", "a.cxxm", "a.c++m", "sketch.ino") ) def test_split_source_files_knows_pre_commit_cpp_types(name): - assert _split_source_files(["-quiet", "a.cpp", name]) == ( + assert split_source_files(["-quiet", "a.cpp", name]) == ( ["-quiet"], ["a.cpp", name], ) @@ -1619,7 +1619,7 @@ def test_split_source_files_keeps_files_with_other_suffixes(tmp_path, monkeypatc monkeypatch.chdir(tmp_path) (tmp_path / "x.metal").write_text("") - assert _split_source_files(["-quiet", "a.cpp", "x.metal", "b.cpp"]) == ( + assert split_source_files(["-quiet", "a.cpp", "x.metal", "b.cpp"]) == ( ["-quiet"], ["a.cpp", "x.metal", "b.cpp"], ) @@ -1629,7 +1629,7 @@ def test_split_source_files_keeps_an_option_value_that_is_a_file(tmp_path, monke monkeypatch.chdir(tmp_path) (tmp_path / "fixes.yaml").write_text("") - assert _split_source_files(["--export-fixes", "fixes.yaml", "a.cpp"]) == ( + assert split_source_files(["--export-fixes", "fixes.yaml", "a.cpp"]) == ( ["--export-fixes", "fixes.yaml"], ["a.cpp"], )