From 1d77aa3032e8575712821149d82e594920f05cee Mon Sep 17 00:00:00 2001 From: Jan Holthuis Date: Fri, 31 Jul 2020 18:19:12 +0200 Subject: [PATCH 1/9] tools: Refactor python clang-format wrapping This merges the clang-format and line-length hooks into a single one. Semantically, this makes way more sense. Also, this prevent multiple attempted commit that are rejected by pre-commit (pre-commit might skip the line-length hook if old clang-format hook already failed). Also, this removes any dependency on git-clang-format and implements the whole mechanism itself. It retrieves all added lines from the unified diff instead and is now capable of taking PRE_COMMIT_FROM_REF into account. By moving the git-related code into a separate githelper library, it's way easier to write additional python wrappers for new hooks, e.g. for clang-tidy. --- .pre-commit-config.yaml | 13 +-- tools/clang_format.py | 124 +++++++++++++++++++++++ tools/clang_format_wrapper.py | 130 ------------------------ tools/githelper.py | 111 ++++++++++++++++++++ tools/line_length.py | 184 ---------------------------------- 5 files changed, 236 insertions(+), 326 deletions(-) create mode 100755 tools/clang_format.py delete mode 100755 tools/clang_format_wrapper.py create mode 100644 tools/githelper.py delete mode 100755 tools/line_length.py diff --git a/.pre-commit-config.yaml b/.pre-commit-config.yaml index 2d17748bee3..ee7c6226318 100644 --- a/.pre-commit-config.yaml +++ b/.pre-commit-config.yaml @@ -78,7 +78,7 @@ repos: hooks: - id: clang-format name: clang-format - entry: tools/clang_format_wrapper.py + entry: tools/clang_format.py require_serial: true stages: - commit @@ -114,14 +114,3 @@ repos: stages: - commit - manual -- repo: local - hooks: - - id: line-length - name: line-length - description: Check for lines longer 100 and brakes them before 80. - entry: ./tools/line_length.py - stages: - - commit - - manual - language: python - files: \.(c|cpp|h)$ diff --git a/tools/clang_format.py b/tools/clang_format.py new file mode 100755 index 00000000000..4447cb70aa2 --- /dev/null +++ b/tools/clang_format.py @@ -0,0 +1,124 @@ +#!/usr/bin/env python3 +# -*- coding: utf-8 -*- +import argparse +import logging +import os +import re +import subprocess +import sys +import tempfile +import typing + +import githelper + + +# We recommend a maximum line length of 80, but do allow up to 100 characters +# if deemed necessary by the developer. Lines that exceed that limit will +# be wrapped after 80 characters automatically. +LINE_LENGTH_THRESHOLD = 100 +BREAK_BEFORE = 80 + + +def get_clang_format_config_with_columnlimit(rootdir, limit): + cpp_file = os.path.join(rootdir, "src/mixxx.cpp") + proc = subprocess.run( + ["clang-format", "--dump-config", cpp_file], + capture_output=True, + text=True, + ) + proc.check_returncode() + return re.sub(r"(ColumnLimit:\s*)\d+", r"\g<1>{}".format(80), proc.stdout,) + + +def run_clang_format_on_lines(rootdir, changed_file, assume_filename=None): + logger = logging.getLogger(__name__) + + line_arguments = [ + "--lines={}:{}".format(start, end) for start, end in changed_file.lines + ] + assert line_arguments + + logger.info("Reformatting %s...", changed_file.filename) + filename = os.path.join(rootdir, changed_file.filename) + cmd = [ + "clang-format", + "--style=file", + "--assume-filename={}".format( + assume_filename if assume_filename else filename + ), + *line_arguments, + ] + + with open(filename) as fp: + logger.debug("Executing: %r", cmd) + proc = subprocess.run(cmd, stdin=fp, capture_output=True, text=True) + try: + proc.check_returncode() + except subprocess.CalledProcessError: + logger.error( + "Error while executing command %s: %s", cmd, proc.stderr, + ) + raise + + if proc.stderr: + logger.error(proc.stderr) + with open(filename, mode="w+") as fp: + fp.write(proc.stdout) + + +def main(argv: typing.Optional[typing.List[str]] = None) -> int: + logging.basicConfig( + format="[%(levelname)s] %(message)s", level=logging.INFO + ) + + logger = logging.getLogger(__name__) + + parser = argparse.ArgumentParser() + parser.add_argument("--from-ref", help="compare against git reference") + parser.add_argument("files", nargs="*", help="only check these files") + args = parser.parse_args(argv) + + if not args.from_ref: + args.from_ref = os.getenv("PRE_COMMIT_FROM_REF") or os.getenv( + "PRE_COMMIT_SOURCE" + ) + + # Filter filenames + rootdir = githelper.get_toplevel_path() + + # First pass: Format added lines using clang-format + logger.info("First pass: Reformat files...") + files_with_added_lines = githelper.get_changed_lines_grouped( + from_ref=args.from_ref, + filter_lines=lambda line: line.added, + include_files=args.files, + ) + for changed_file in files_with_added_lines: + run_clang_format_on_lines(rootdir, changed_file) + + # Second pass: Wrap long added lines using clang-format + logger.info("Second pass: Reformat files with long lines...") + files_with_long_added_lines = githelper.get_changed_lines_grouped( + from_ref=args.from_ref, + filter_lines=lambda line: line.added + and LINE_LENGTH_THRESHOLD < (len(line.text) - 1), + include_files=args.files, + ) + config = get_clang_format_config_with_columnlimit(rootdir, BREAK_BEFORE) + with tempfile.TemporaryDirectory(prefix="clang-format") as tempdir: + # Create temporary config with ColumnLimit enabled + configfile = os.path.join(tempdir, ".clang-format") + with open(configfile, mode="w") as configfp: + configfp.write(config) + + for changed_file in files_with_long_added_lines: + run_clang_format_on_lines( + rootdir, + changed_file, + assume_filename=os.path.join(tempdir, changed_file.filename), + ) + return 0 + + +if __name__ == "__main__": + sys.exit(main()) diff --git a/tools/clang_format_wrapper.py b/tools/clang_format_wrapper.py deleted file mode 100755 index b2fc23454cf..00000000000 --- a/tools/clang_format_wrapper.py +++ /dev/null @@ -1,130 +0,0 @@ -#!/usr/bin/env python -"""Wrapper script for `git-clang-format` to avoid race conditions when -merging/rebasing CPP files with merge conflicts.""" -from collections import namedtuple -from os import environ as env -from subprocess import check_output -from typing import List, Iterable -import argparse -import enum -import logging -import sys - -logger = logging.getLogger(__name__) - - -class GitState(enum.Enum): - """ - Excerpt from https://git-scm.com/docs/git-status - - For paths with merge conflicts, X and Y show the modification states - of each side of the merge. For paths that do not have merge conflicts, - X shows the status of the index, and Y shows the status of the work - tree. For untracked paths, XY are ??. Other status codes can be - interpreted as follows: - M = modified - A = added - D = deleted - R = renamed - C = copied - U = updated but unmerged - """ - - ADDED = "A" - COPIED = "C" - DELETED = "D" - MODIFIED = "M" - RENAMED = "R" - UNMODIFIED = " " - UNTRACKED = "?" - UPDATED_BUT_UNMERGED = "U" - - -GitEdit = namedtuple("GitEdit", ["x", "y", "path"]) - - -def get_git_status() -> Iterable[GitEdit]: - """Inspect `git status` output, return GitEdits generator.""" - - command = ["git", "status", "--porcelain", "--untracked-files=no"] - logger.debug("Executing `%s`", " ".join(command)) - git_status_output = check_output(args=command, text=True) - - edits = [ - edit for edit in git_status_output.rstrip().split("\n") if edit != "" - ] - logger.debug( - "Found %d path(s) with state change in GIT status output", len(edits) - ) - - if edits: - logger.debug("Pending GIT edits:\n%s", "\n".join(edits)) - - return ( - git_edit - for git_edit in ( - GitEdit(edit[0:1], edit[1:2], edit[3:]) for edit in edits - ) - if git_edit.x in (GitState.MODIFIED.value, GitState.ADDED.value) - ) - - -def run_clang_format(file_list: List[str]) -> bool: - """Execute git-clang-format for provided `file_list`. - - Return True if `git-clang-format` has errors or has modified any file, - i.e. if it failed the formatting test. - """ - if not file_list: - logger.info("No file to be formatted, exiting") - return - - command = ["git-clang-format"] - - src_commit = env.get("PRE_COMMIT_FROM_REF") or env.get("PRE_COMMIT_SOURCE") - - if src_commit: - command.append(src_commit) - - command.append("--") - command.extend(file_list) - - logger.info("Executing `%s`", " ".join(command)) - out = check_output(command, text=True) - logger.debug(out) - - return False if out == "clang-format did not modify any files\n" else True - - -def main(): - # Setup CLI argument parser - logging.basicConfig( - format="[%(levelname)s] %(message)s", level=logging.DEBUG - ) - parser = argparse.ArgumentParser() - parser.add_argument( - "files", nargs="*", help="Restrict formatting to these files" - ) - args = parser.parse_args() - logger.debug("Wrapper called with `files` arguments: %s", args.files) - - # Inspect GIT status output and filter paths that must be clang-formatted - paths_to_be_clang_formatted = [ - status.path for status in get_git_status() if status.path in args.files - ] - logger.debug( - "Found %d staged path(s) to be clang-formatted", - len(paths_to_be_clang_formatted), - ) - - if paths_to_be_clang_formatted: - logger.debug("Paths:\n%s", "\n".join(paths_to_be_clang_formatted)) - - if run_clang_format(paths_to_be_clang_formatted): - return 1 - - return 0 - - -if __name__ == "__main__": - sys.exit(main()) diff --git a/tools/githelper.py b/tools/githelper.py new file mode 100644 index 00000000000..b7eb1656b1c --- /dev/null +++ b/tools/githelper.py @@ -0,0 +1,111 @@ +import itertools +import re +import logging +import subprocess +import typing + + +Line = typing.NamedTuple( + "Line", + [("sourcefile", str), ("number", int), ("text", str), ("added", bool)], +) +FileLines = typing.NamedTuple( + "FileLines", + [("filename", str), ("lines", typing.Sequence[typing.Tuple[int, int]])], +) + + +def get_toplevel_path() -> str: + logger = logging.getLogger(__name__) + + cmd = ["git", "rev-parse", "--show-toplevel"] + logger.debug("Executing: %r", cmd) + return subprocess.check_output(cmd, text=True).strip() + + +def get_changed_lines( + from_ref=None, filter_lines=None, include_files=None +) -> typing.Iterable[Line]: + """Inspect `git diff` output, yields changed lines.""" + + logger = logging.getLogger(__name__) + + cmd = ["git", "diff", "--unified=0", from_ref if from_ref else "HEAD"] + if include_files: + cmd.extend(["--", *include_files]) + logger.debug("Executing: %r", cmd) + proc = subprocess.run(cmd, capture_output=True) + proc.check_returncode() + current_file = None + lines_left = 0 + for line in proc.stdout.decode(errors="replace").splitlines(): + match_file = re.match(r"^\+\+\+ b/(.*)$", line) + if match_file: + current_file = match_file.group(1) + lines_left = 0 + continue + + match_lineno = re.match( + r"^@@ -(\d+(?:,\d+)?) \+([0-9]+(?:,[0-9]+)?) @@", line + ) + if match_lineno: + start_removed, _, length_removed = match_lineno.group(1).partition( + "," + ) + start_added, _, length_added = match_lineno.group(2).partition(",") + lineno_removed = int(start_removed) + lineno_added = int(start_added) + lines_left = int(length_removed) if length_removed else 1 + lines_left += int(length_added) if length_added else 1 + continue + + if lines_left and line: + lines_left -= 1 + + assert line[0] in ("+", "-") + if line.startswith("+"): + lineno_added += 1 + current_lineno = lineno_added + else: + lineno_removed += 1 + current_lineno = lineno_removed + + lineobj = Line( + sourcefile=current_file, + number=current_lineno - 1, + text=line[1:], + added=line.startswith("+"), + ) + + if filter_lines is None or filter_lines(lineobj): + yield lineobj + + +def get_changed_lines_grouped( + from_ref=None, filter_lines=None, include_files=None +) -> typing.Iterable[FileLines]: + """Inspect `git diff` output, yields changed lines grouped by file.""" + + lines = get_changed_lines(from_ref, filter_lines, include_files) + for filename, file_lines in itertools.groupby( + lines, key=lambda line: line.sourcefile + ): + grouped_linenumbers = [] + start_linenumber = None + last_linenumber = None + for line in file_lines: + if None not in (start_linenumber, last_linenumber): + if line.number != last_linenumber + 1: + grouped_linenumbers.append( + (start_linenumber, last_linenumber) + ) + + if start_linenumber is None: + start_linenumber = line.number + last_linenumber = line.number + + if None not in (start_linenumber, last_linenumber): + grouped_linenumbers.append((start_linenumber, last_linenumber)) + + if grouped_linenumbers: + yield FileLines(filename, grouped_linenumbers) diff --git a/tools/line_length.py b/tools/line_length.py deleted file mode 100755 index dd816384ade..00000000000 --- a/tools/line_length.py +++ /dev/null @@ -1,184 +0,0 @@ -#!/usr/bin/env python3 -# -*- coding: utf-8 -*- -import argparse -import re -import logging -import os -import itertools -import subprocess -import tempfile -import typing - -# We recommend a maximum line length of 80, but do allow up to 100 characters -# if deemed necessary by the developer. Lines that exceed that limit will -# be wrapped after 80 characters automatically. -LINE_LENGTH_THRESHOLD = 100 -BREAK_BEFORE = 80 - -Line = typing.NamedTuple( - "Line", [("sourcefile", str), ("number", int), ("text", str)] -) -LineGenerator = typing.Generator[Line, None, None] -FileLines = typing.NamedTuple( - "FileLines", - [("filename", str), ("lines", typing.Sequence[typing.Tuple[int, int]])], -) - - -def get_git_added_lines() -> LineGenerator: - proc = subprocess.run( - ["git", "diff", "--cached", "--unified=0"], capture_output=True - ) - proc.check_returncode() - current_file = None - current_lineno = None - lines_left = 0 - for line in proc.stdout.decode(errors="replace").splitlines(): - match_file = re.match(r"^\+\+\+ b/(.*)$", line) - if match_file: - current_file = match_file.group(1) - lines_left = 0 - continue - - match_lineno = re.match( - r"^@@ -\d+(?:,\d+)? \+([0-9]+(?:,[0-9]+)?) @@", line - ) - if match_lineno: - start, _, length = match_lineno.group(1).partition(",") - current_lineno = int(start) - lines_left = 1 - if length: - lines_left += int(length) - continue - - if lines_left and line.startswith("+"): - yield Line( - sourcefile=current_file, number=current_lineno, text=line[1:] - ) - lines_left -= 1 - current_lineno += 1 - - -def group_lines( - lines: LineGenerator, -) -> typing.Generator[FileLines, None, None]: - for filename, file_lines in itertools.groupby( - lines, key=lambda line: line.sourcefile - ): - grouped_linenumbers = [] - start_linenumber = None - last_linenumber = None - for line in file_lines: - if None not in (start_linenumber, last_linenumber): - if line.number != last_linenumber + 1: - grouped_linenumbers.append( - (start_linenumber, last_linenumber) - ) - - if start_linenumber is None: - start_linenumber = line.number - last_linenumber = line.number - - if None not in (start_linenumber, last_linenumber): - grouped_linenumbers.append((start_linenumber, last_linenumber)) - - if grouped_linenumbers: - yield FileLines(filename, grouped_linenumbers) - - -def main(argv: typing.Optional[typing.List[str]] = None) -> int: - logging.basicConfig() - logger = logging.getLogger(__name__) - - import sys - - print(sys.argv) - - parser = argparse.ArgumentParser() - parser.add_argument("files", nargs="*", help="only check these files") - args = parser.parse_args(argv) - print(args) - - all_lines = get_git_added_lines() - - # Filter filenames - rootdir = os.path.join(os.path.dirname(os.path.abspath(__file__)), "..") - if args.files: - files = [ - os.path.abspath(os.path.join(rootdir, filename)) - for filename in args.files - ] - all_lines = ( - line - for line in all_lines - if os.path.abspath(os.path.join(line.sourcefile)) in files - ) - - # Only keep long lines - long_lines = ( - line - for line in all_lines - if (len(line.text) - 1) >= LINE_LENGTH_THRESHOLD - ) - changed_files = group_lines(long_lines) - - cpp_file = os.path.join(rootdir, "src/mixxx.cpp") - proc = subprocess.run( - ["clang-format", "--dump-config", cpp_file], - capture_output=True, - text=True, - ) - proc.check_returncode() - - with tempfile.TemporaryDirectory(prefix="clang-format") as tempdir: - # Create temporary config with ColumnLimit enabled - configfile = os.path.join(tempdir, ".clang-format") - with open(configfile, mode="w") as configfp: - configfp.write( - re.sub( - r"(ColumnLimit:\s*)\d+", - r"\g<1>{}".format(BREAK_BEFORE), - proc.stdout, - ) - ) - - for changed_file in changed_files: - line_arguments = [ - "--lines={}:{}".format(start, end) - for start, end in changed_file.lines - ] - - if not line_arguments: - continue - - cmd = [ - "clang-format", - "--style=file", - "--assume-filename={}".format( - os.path.join(tempdir, changed_file.filename) - ), - *line_arguments, - ] - - filename = os.path.join(rootdir, changed_file.filename) - with open(filename) as fp: - proc = subprocess.run( - cmd, stdin=fp, capture_output=True, text=True - ) - try: - proc.check_returncode() - except subprocess.CalledProcessError: - logger.error( - "Error while executing command %s: %s", cmd, proc.stderr, - ) - return 1 - - if proc.stderr: - print(proc.stderr) - with open(filename, mode="w+") as fp: - fp.write(proc.stdout) - return 0 - - -if __name__ == "__main__": - exit(main()) From 5589ce8cac60eab8886efa5e234bfcd7a1565b9e Mon Sep 17 00:00:00 2001 From: Jan Holthuis Date: Sat, 1 Aug 2020 12:46:27 +0200 Subject: [PATCH 2/9] pre-commit: Improve documentation for clang-format hook --- .pre-commit-config.yaml | 1 + tools/clang_format.py | 4 ++-- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/.pre-commit-config.yaml b/.pre-commit-config.yaml index ee7c6226318..fd9e1f3eeb5 100644 --- a/.pre-commit-config.yaml +++ b/.pre-commit-config.yaml @@ -78,6 +78,7 @@ repos: hooks: - id: clang-format name: clang-format + description: "Run clang-format in two passes (reformat, then break long lines)" entry: tools/clang_format.py require_serial: true stages: diff --git a/tools/clang_format.py b/tools/clang_format.py index 4447cb70aa2..80aa419c2f0 100755 --- a/tools/clang_format.py +++ b/tools/clang_format.py @@ -87,7 +87,7 @@ def main(argv: typing.Optional[typing.List[str]] = None) -> int: rootdir = githelper.get_toplevel_path() # First pass: Format added lines using clang-format - logger.info("First pass: Reformat files...") + logger.info("First pass: Reformatting added/changed lines...") files_with_added_lines = githelper.get_changed_lines_grouped( from_ref=args.from_ref, filter_lines=lambda line: line.added, @@ -97,7 +97,7 @@ def main(argv: typing.Optional[typing.List[str]] = None) -> int: run_clang_format_on_lines(rootdir, changed_file) # Second pass: Wrap long added lines using clang-format - logger.info("Second pass: Reformat files with long lines...") + logger.info("Second pass: Breaking long added/changed lines...") files_with_long_added_lines = githelper.get_changed_lines_grouped( from_ref=args.from_ref, filter_lines=lambda line: line.added From 1e7698d781179b4c0a05ee40d8245e7fd4896bd6 Mon Sep 17 00:00:00 2001 From: Jan Holthuis Date: Mon, 3 Aug 2020 21:08:37 +0200 Subject: [PATCH 3/9] tools: Add various improvements for clang_format.py/githelper.py --- tools/clang_format.py | 33 +++++++++++++++++++-------------- tools/githelper.py | 36 +++++++++++++++++++++++++++--------- 2 files changed, 46 insertions(+), 23 deletions(-) diff --git a/tools/clang_format.py b/tools/clang_format.py index 80aa419c2f0..9c26c76a672 100755 --- a/tools/clang_format.py +++ b/tools/clang_format.py @@ -20,6 +20,7 @@ def get_clang_format_config_with_columnlimit(rootdir, limit): + """Create a temporary config with ColumnLimit set to 80.""" cpp_file = os.path.join(rootdir, "src/mixxx.cpp") proc = subprocess.run( ["clang-format", "--dump-config", cpp_file], @@ -27,24 +28,32 @@ def get_clang_format_config_with_columnlimit(rootdir, limit): text=True, ) proc.check_returncode() - return re.sub(r"(ColumnLimit:\s*)\d+", r"\g<1>{}".format(80), proc.stdout,) + return re.sub( + r"(ColumnLimit:\s*)\d+", r"\g<1>{}".format(BREAK_BEFORE), proc.stdout + ) -def run_clang_format_on_lines(rootdir, changed_file, assume_filename=None): +def run_clang_format_on_lines(rootdir, file_to_format, stylepath=None): logger = logging.getLogger(__name__) line_arguments = [ - "--lines={}:{}".format(start, end) for start, end in changed_file.lines + "--lines={}:{}".format(start, end) + for start, end in file_to_format.lines ] assert line_arguments - logger.info("Reformatting %s...", changed_file.filename) - filename = os.path.join(rootdir, changed_file.filename) + logger.info("Reformatting %s...", file_to_format.filename) + filename = os.path.join(rootdir, file_to_format.filename) cmd = [ "clang-format", "--style=file", + # The --assume-filename argument sets the path for the .clang-format + # config file implcitly by assuming a different location of the file to + # format "--assume-filename={}".format( - assume_filename if assume_filename else filename + os.path.join( + stylepath if stylepath else rootdir, file_to_format.filename + ) ), *line_arguments, ] @@ -86,7 +95,7 @@ def main(argv: typing.Optional[typing.List[str]] = None) -> int: # Filter filenames rootdir = githelper.get_toplevel_path() - # First pass: Format added lines using clang-format + # First pass: Format added/changed lines using clang-format logger.info("First pass: Reformatting added/changed lines...") files_with_added_lines = githelper.get_changed_lines_grouped( from_ref=args.from_ref, @@ -96,12 +105,12 @@ def main(argv: typing.Optional[typing.List[str]] = None) -> int: for changed_file in files_with_added_lines: run_clang_format_on_lines(rootdir, changed_file) - # Second pass: Wrap long added lines using clang-format + # Second pass: Wrap long added/changed lines using clang-format logger.info("Second pass: Breaking long added/changed lines...") files_with_long_added_lines = githelper.get_changed_lines_grouped( from_ref=args.from_ref, filter_lines=lambda line: line.added - and LINE_LENGTH_THRESHOLD < (len(line.text) - 1), + and len(line.text) > LINE_LENGTH_THRESHOLD, include_files=args.files, ) config = get_clang_format_config_with_columnlimit(rootdir, BREAK_BEFORE) @@ -112,11 +121,7 @@ def main(argv: typing.Optional[typing.List[str]] = None) -> int: configfp.write(config) for changed_file in files_with_long_added_lines: - run_clang_format_on_lines( - rootdir, - changed_file, - assume_filename=os.path.join(tempdir, changed_file.filename), - ) + run_clang_format_on_lines(rootdir, changed_file, stylepath=tempdir) return 0 diff --git a/tools/githelper.py b/tools/githelper.py index b7eb1656b1c..29e9ee0a4ea 100644 --- a/tools/githelper.py +++ b/tools/githelper.py @@ -26,41 +26,52 @@ def get_toplevel_path() -> str: def get_changed_lines( from_ref=None, filter_lines=None, include_files=None ) -> typing.Iterable[Line]: - """Inspect `git diff` output, yields changed lines.""" + """Inspect `git diff-index` output, yields changed lines.""" logger = logging.getLogger(__name__) - cmd = ["git", "diff", "--unified=0", from_ref if from_ref else "HEAD"] + cmd = [ + "git", + "diff-index", + "--patch", + "--unified=0", + from_ref if from_ref else "HEAD", + ] if include_files: cmd.extend(["--", *include_files]) logger.debug("Executing: %r", cmd) proc = subprocess.run(cmd, capture_output=True) proc.check_returncode() current_file = None - lines_left = 0 + hunk_lines_left = 0 for line in proc.stdout.decode(errors="replace").splitlines(): match_file = re.match(r"^\+\+\+ b/(.*)$", line) if match_file: + # Current line contains a diff filename + assert hunk_lines_left == 0 current_file = match_file.group(1) - lines_left = 0 continue match_lineno = re.match( r"^@@ -(\d+(?:,\d+)?) \+([0-9]+(?:,[0-9]+)?) @@", line ) if match_lineno: + # Current line contains a hunk header + assert current_file is not None + assert hunk_lines_left == 0 start_removed, _, length_removed = match_lineno.group(1).partition( "," ) start_added, _, length_added = match_lineno.group(2).partition(",") lineno_removed = int(start_removed) lineno_added = int(start_added) - lines_left = int(length_removed) if length_removed else 1 - lines_left += int(length_added) if length_added else 1 + hunk_lines_left = int(length_removed) if length_removed else 1 + hunk_lines_left += int(length_added) if length_added else 1 continue - if lines_left and line: - lines_left -= 1 + if hunk_lines_left and line: + # Current line contains an added/removed line + hunk_lines_left -= 1 assert line[0] in ("+", "-") if line.startswith("+"): @@ -80,11 +91,18 @@ def get_changed_lines( if filter_lines is None or filter_lines(lineobj): yield lineobj + # If we reach this part, the line does not contain a diff filename or a + # hunk header and does not belog to a hunk. This means that this line + # will be ignored implicitly. + + # Make sure we really parsed all lines from the last hunk + assert hunk_lines_left == 0 + def get_changed_lines_grouped( from_ref=None, filter_lines=None, include_files=None ) -> typing.Iterable[FileLines]: - """Inspect `git diff` output, yields changed lines grouped by file.""" + """Inspect `git diff-index` output, yields lines grouped by file.""" lines = get_changed_lines(from_ref, filter_lines, include_files) for filename, file_lines in itertools.groupby( From c91ed3f1484302da7a88dfed87726666eed38046 Mon Sep 17 00:00:00 2001 From: Jan Holthuis Date: Tue, 4 Aug 2020 17:21:51 +0200 Subject: [PATCH 4/9] tools/clang-format.py: Use plain "w" mode for writing files --- tools/clang_format.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tools/clang_format.py b/tools/clang_format.py index 9c26c76a672..ba6fd803b20 100755 --- a/tools/clang_format.py +++ b/tools/clang_format.py @@ -71,7 +71,7 @@ def run_clang_format_on_lines(rootdir, file_to_format, stylepath=None): if proc.stderr: logger.error(proc.stderr) - with open(filename, mode="w+") as fp: + with open(filename, mode="w") as fp: fp.write(proc.stdout) From 3babcd7c472f41e031376d678097a2005da99d93 Mon Sep 17 00:00:00 2001 From: Jan Holthuis Date: Tue, 4 Aug 2020 17:23:15 +0200 Subject: [PATCH 5/9] tools/clang-format.py: Add support for --to-ref argument --- tools/clang_format.py | 10 +++++++++- tools/githelper.py | 19 +++++++++---------- 2 files changed, 18 insertions(+), 11 deletions(-) diff --git a/tools/clang_format.py b/tools/clang_format.py index ba6fd803b20..d434ad49ca0 100755 --- a/tools/clang_format.py +++ b/tools/clang_format.py @@ -83,7 +83,8 @@ def main(argv: typing.Optional[typing.List[str]] = None) -> int: logger = logging.getLogger(__name__) parser = argparse.ArgumentParser() - parser.add_argument("--from-ref", help="compare against git reference") + parser.add_argument("--from-ref", help="use changes changes since commit") + parser.add_argument("--to-ref", help="use changes until commit") parser.add_argument("files", nargs="*", help="only check these files") args = parser.parse_args(argv) @@ -92,6 +93,11 @@ def main(argv: typing.Optional[typing.List[str]] = None) -> int: "PRE_COMMIT_SOURCE" ) + if not args.to_ref: + args.to_ref = os.getenv("PRE_COMMIT_TO_REF") or os.getenv( + "PRE_COMMIT_ORIGIN" + ) + # Filter filenames rootdir = githelper.get_toplevel_path() @@ -99,6 +105,7 @@ def main(argv: typing.Optional[typing.List[str]] = None) -> int: logger.info("First pass: Reformatting added/changed lines...") files_with_added_lines = githelper.get_changed_lines_grouped( from_ref=args.from_ref, + to_ref=args.to_ref, filter_lines=lambda line: line.added, include_files=args.files, ) @@ -109,6 +116,7 @@ def main(argv: typing.Optional[typing.List[str]] = None) -> int: logger.info("Second pass: Breaking long added/changed lines...") files_with_long_added_lines = githelper.get_changed_lines_grouped( from_ref=args.from_ref, + to_ref=args.to_ref, filter_lines=lambda line: line.added and len(line.text) > LINE_LENGTH_THRESHOLD, include_files=args.files, diff --git a/tools/githelper.py b/tools/githelper.py index 29e9ee0a4ea..8a2cdd7a209 100644 --- a/tools/githelper.py +++ b/tools/githelper.py @@ -24,19 +24,18 @@ def get_toplevel_path() -> str: def get_changed_lines( - from_ref=None, filter_lines=None, include_files=None + from_ref=None, to_ref=None, filter_lines=None, include_files=None ) -> typing.Iterable[Line]: """Inspect `git diff-index` output, yields changed lines.""" logger = logging.getLogger(__name__) - cmd = [ - "git", - "diff-index", - "--patch", - "--unified=0", - from_ref if from_ref else "HEAD", - ] + if to_ref: + changeset = "{}...{}".format(from_ref if from_ref else "HEAD", to_ref) + else: + changeset = from_ref if from_ref else "HEAD" + + cmd = ["git", "diff", "--patch", "--unified=0", changeset] if include_files: cmd.extend(["--", *include_files]) logger.debug("Executing: %r", cmd) @@ -100,11 +99,11 @@ def get_changed_lines( def get_changed_lines_grouped( - from_ref=None, filter_lines=None, include_files=None + from_ref=None, to_ref=None, filter_lines=None, include_files=None ) -> typing.Iterable[FileLines]: """Inspect `git diff-index` output, yields lines grouped by file.""" - lines = get_changed_lines(from_ref, filter_lines, include_files) + lines = get_changed_lines(from_ref, to_ref, filter_lines, include_files) for filename, file_lines in itertools.groupby( lines, key=lambda line: line.sourcefile ): From ff15b5bb88a69bfed30a02ffcd38fa5b651a6089 Mon Sep 17 00:00:00 2001 From: Jan Holthuis Date: Tue, 4 Aug 2020 17:48:13 +0200 Subject: [PATCH 6/9] pre-commit: Do not format JS files using clang-format --- .pre-commit-config.yaml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.pre-commit-config.yaml b/.pre-commit-config.yaml index fd9e1f3eeb5..332b02c4956 100644 --- a/.pre-commit-config.yaml +++ b/.pre-commit-config.yaml @@ -85,7 +85,7 @@ repos: - commit - manual language: python - files: \.(c|cc|cxx|cpp|frag|glsl|h|hpp|hxx|ih|ispc|ipp|java|js|m|mm|proto|vert)$ + files: \.(c|cc|cxx|cpp|frag|glsl|h|hpp|hxx|ih|ispc|ipp|java|m|mm|proto|vert)$ additional_dependencies: - clang-format - repo: https://github.com/psf/black From eae9aba66e932c4f3ba46d46db6a5cba4b9a9867 Mon Sep 17 00:00:00 2001 From: Jan Holthuis Date: Wed, 5 Aug 2020 10:52:52 +0200 Subject: [PATCH 7/9] tools/githelper: Add comment regarding unstaged changes --- tools/githelper.py | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/tools/githelper.py b/tools/githelper.py index 8a2cdd7a209..c526db3d0e9 100644 --- a/tools/githelper.py +++ b/tools/githelper.py @@ -35,6 +35,11 @@ def get_changed_lines( else: changeset = from_ref if from_ref else "HEAD" + # We're using the pre-commit framework which stashes all unstaged changes + # before running the pre-commit hooks, so we don't need to add `--cached` + # here. Also, if we run 2 hooks that modify the files, the second hook + # should work on the diff that includes the unstaged changes made by the + # first hook, not the original diff. cmd = ["git", "diff", "--patch", "--unified=0", changeset] if include_files: cmd.extend(["--", *include_files]) From f916521f93ad429288e446aff4ccc86dc72c5739 Mon Sep 17 00:00:00 2001 From: Jan Holthuis Date: Sat, 1 Aug 2020 11:52:08 +0200 Subject: [PATCH 8/9] pre-commit: Add clang-tidy support --- .clang-tidy | 7 ++++ .pre-commit-config.yaml | 14 ++++++++ CMakeLists.txt | 7 ++++ tools/clang_tidy.py | 79 +++++++++++++++++++++++++++++++++++++++++ 4 files changed, 107 insertions(+) create mode 100644 .clang-tidy create mode 100755 tools/clang_tidy.py diff --git a/.clang-tidy b/.clang-tidy new file mode 100644 index 00000000000..69745b7cb5f --- /dev/null +++ b/.clang-tidy @@ -0,0 +1,7 @@ +--- +Checks: '-*,modernize-use-nullptr,modernize-use-auto' +WarningsAsErrors: '' +HeaderFilterRegex: '' +AnalyzeTemporaryDtors: false +FormatStyle: none +... diff --git a/.pre-commit-config.yaml b/.pre-commit-config.yaml index 332b02c4956..a205a1a29e7 100644 --- a/.pre-commit-config.yaml +++ b/.pre-commit-config.yaml @@ -74,6 +74,20 @@ repos: stages: - commit - manual +- repo: local + hooks: + - id: clang-tidy + name: clang-tidy + entry: tools/clang_tidy.py + require_serial: true + verbose: true + stages: + - commit + - manual + language: system + files: \.(c|cc|cxx|cpp|frag|glsl|h|hpp|hxx|ih|ispc|ipp|java|js|m|mm|proto|vert)$ + additional_dependencies: + - clang-tidy - repo: local hooks: - id: clang-format diff --git a/CMakeLists.txt b/CMakeLists.txt index 235a620616e..dc7444d4ff2 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -319,6 +319,13 @@ if(CMAKE_VERSION VERSION_LESS "3.7.0") set(CMAKE_INCLUDE_CURRENT_DIR ON) endif() +# Export compile-commands.json and symlink it into the source directory (needed +# for clang-tidy pre-commit hook) +set(CMAKE_EXPORT_COMPILE_COMMANDS ON) +file(CREATE_LINK + ${CMAKE_BINARY_DIR}/compile_commands.json + ${CMAKE_SOURCE_DIR}/compile_commands.json SYMBOLIC) + # Mixxx itself add_library(mixxx-lib STATIC EXCLUDE_FROM_ALL src/analyzer/analyzerbeats.cpp diff --git a/tools/clang_tidy.py b/tools/clang_tidy.py new file mode 100755 index 00000000000..dd6d5fedcc9 --- /dev/null +++ b/tools/clang_tidy.py @@ -0,0 +1,79 @@ +#!/usr/bin/env python3 +# -*- coding: utf-8 -*- +import argparse +import logging +import json +import os +import subprocess +import sys +import typing + +import githelper + + +def run_clang_tidy_on_lines(rootdir, changed_files): + logger = logging.getLogger(__name__) + + files = [] + lines = [] + for changed_file in changed_files: + logger.debug("Tidying up %s...", changed_file.filename) + filename = os.path.join(rootdir, changed_file.filename) + files.append(filename) + lines.append({"name": filename, "lines": changed_file.lines}) + + cmd = [ + "clang-tidy", + "--fix", + "--line-filter={}".format(json.dumps(lines)), + *files, + ] + + proc = subprocess.run(cmd, capture_output=True, text=True) + try: + proc.check_returncode() + except subprocess.CalledProcessError: + logger.error( + "Error while executing command %s: %s", cmd, proc.stderr, + ) + raise + + +def main(argv: typing.Optional[typing.List[str]] = None) -> int: + logging.basicConfig( + format="[%(levelname)s] %(message)s", level=logging.INFO + ) + + logger = logging.getLogger(__name__) + + parser = argparse.ArgumentParser() + parser.add_argument("--from-ref", help="compare against git reference") + parser.add_argument("files", nargs="*", help="only check these files") + args = parser.parse_args(argv) + + if not args.from_ref: + args.from_ref = os.getenv("PRE_COMMIT_FROM_REF") or os.getenv( + "PRE_COMMIT_SOURCE" + ) + + rootdir = githelper.get_toplevel_path() + + # clang-tidy requires a compile_commands.json file to work correctly + # If it doesn't exist, this hook should be skipped without failing. + if not os.path.exists(os.path.join(rootdir, "compile_commands.json")): + logger.warning("compile-commands.json not found, skipping hook...") + return 0 + + # Detect added/changed lines and run clang-tidy on them + files_with_added_lines = githelper.get_changed_lines_grouped( + from_ref=args.from_ref, + filter_lines=lambda line: line.added, + include_files=args.files, + ) + run_clang_tidy_on_lines(rootdir, files_with_added_lines) + + return 0 + + +if __name__ == "__main__": + sys.exit(main()) From c58faefd858f0a81948803e41c4436edad05bb1c Mon Sep 17 00:00:00 2001 From: Jan Holthuis Date: Tue, 4 Aug 2020 18:23:58 +0200 Subject: [PATCH 9/9] tools/clang_tidy.py: Add support for PRE_COMMIT_TO_REF --- tools/clang_tidy.py | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/tools/clang_tidy.py b/tools/clang_tidy.py index dd6d5fedcc9..13513971a00 100755 --- a/tools/clang_tidy.py +++ b/tools/clang_tidy.py @@ -56,6 +56,11 @@ def main(argv: typing.Optional[typing.List[str]] = None) -> int: "PRE_COMMIT_SOURCE" ) + if not args.to_ref: + args.from_ref = os.getenv("PRE_COMMIT_TO_REF") or os.getenv( + "PRE_COMMIT_ORIGIN" + ) + rootdir = githelper.get_toplevel_path() # clang-tidy requires a compile_commands.json file to work correctly @@ -67,6 +72,7 @@ def main(argv: typing.Optional[typing.List[str]] = None) -> int: # Detect added/changed lines and run clang-tidy on them files_with_added_lines = githelper.get_changed_lines_grouped( from_ref=args.from_ref, + to_ref=args.to_ref, filter_lines=lambda line: line.added, include_files=args.files, )