diff --git a/.github/workflows/sep-tests.yml b/.github/workflows/sep-tests.yml index d86c3c57ca..635a36472e 100644 --- a/.github/workflows/sep-tests.yml +++ b/.github/workflows/sep-tests.yml @@ -134,15 +134,17 @@ jobs: if [[ "${GITHUB_EVENT_NAME}" == "pull_request" || "${GITHUB_EVENT_NAME}" == "pull_request_target" ]]; then changed_files=$(mktemp) trap 'rm -f "$changed_files"' EXIT - git diff --name-only ${{ github.event.pull_request.base.sha }}...${{ github.event.pull_request.head.sha }} \ - | while read -r file; do + git diff --name-only -z ${{ github.event.pull_request.base.sha }}...${{ github.event.pull_request.head.sha }} \ + | while IFS= read -r -d '' file; do if [[ -f "$file" ]]; then printf '%s\0' "$file" fi done > "$changed_files" echo "Changed files to run lefthook on:" if [[ -s "$changed_files" ]]; then - tr '\0' '\n' < "$changed_files" | sed 's/^/ /' + while IFS= read -r -d '' file; do + printf ' %q\n' "$file" + done < "$changed_files" else echo " (none — lefthook will be a no-op)" fi diff --git a/.github/workflows/tests.yml b/.github/workflows/tests.yml index 14e7ac4e15..47e0e47a62 100644 --- a/.github/workflows/tests.yml +++ b/.github/workflows/tests.yml @@ -98,15 +98,17 @@ jobs: if [[ "${GITHUB_EVENT_NAME}" == "pull_request" || "${GITHUB_EVENT_NAME}" == "pull_request_target" ]]; then changed_files=$(mktemp) trap 'rm -f "$changed_files"' EXIT - git diff --name-only ${{ github.event.pull_request.base.sha }}...${{ github.event.pull_request.head.sha }} \ - | while read -r file; do + git diff --name-only -z ${{ github.event.pull_request.base.sha }}...${{ github.event.pull_request.head.sha }} \ + | while IFS= read -r -d '' file; do if [[ -f "$file" ]]; then printf '%s\0' "$file" fi done > "$changed_files" echo "Changed files to run lefthook on:" if [[ -s "$changed_files" ]]; then - tr '\0' '\n' < "$changed_files" | sed 's/^/ /' + while IFS= read -r -d '' file; do + printf ' %q\n' "$file" + done < "$changed_files" else echo " (none — lefthook will be a no-op)" fi diff --git a/lefthook.yml b/lefthook.yml index 54efbdd627..4b751e9b6e 100644 --- a/lefthook.yml +++ b/lefthook.yml @@ -70,32 +70,46 @@ pre-commit: else gofmt -w {staged_files} fi - - name: web-prettier - # prettier plugins removed from .prettierrc, so this job needs no node_modules. - glob: "web/**/*.{css,less,json,js,jsx,ts,tsx}" - stage_fixed: true - run: | - if [ "${LEFTHOOK_CHECK_ONLY:-}" = "1" ]; then - cd web && printf '%s\n' {staged_files} | sed 's|^web/||' | xargs npx prettier --check --ignore-unknown - else - cd web && printf '%s\n' {staged_files} | sed 's|^web/||' | xargs npx prettier --write --ignore-unknown - fi - - name: web-eslint - # Only web-eslint needs node_modules (for @typescript-eslint/* plugins). - # web-prettier has no npm deps, so there is no race on node_modules. - glob: "web/**/*.{js,jsx,ts,tsx}" - stage_fixed: true - run: | - if [ ! -f web/node_modules/.package-lock.json ]; then - echo '==> web/node_modules missing or incomplete; running npm ci --prefix web' - rm -rf web/node_modules - npm ci --prefix web --no-audit --no-fund - fi - if [ "${LEFTHOOK_CHECK_ONLY:-}" = "1" ]; then - cd web && printf '%s\n' {staged_files} | sed 's|^web/||' | xargs npx eslint - else - cd web && printf '%s\n' {staged_files} | sed 's|^web/||' | xargs npx eslint --fix - fi + - name: web-checks + group: + piped: true + jobs: + - name: web-deps + glob: "web/**/*.{css,less,json,js,jsx,ts,tsx}" + run: | + # Ensure web/node_modules includes the tooling needed by the hook jobs. + # CI runners don't run `npm install` before lefthook, and `npx` without + # local node_modules fetches the latest packages from the registry - which + # breaks because the pinned prettier plugins + # (prettier-plugin-organize-imports, prettier-plugin-packagejson) aren't + # auto-resolved and ESLint 10 requires eslint.config.js. + if [ ! -f web/node_modules/.package-lock.json ] \ + || [ ! -d web/node_modules/prettier-plugin-organize-imports ] \ + || [ ! -d web/node_modules/@typescript-eslint/eslint-plugin ]; then + echo '==> web/node_modules missing required hook dependencies; running npm ci --prefix web --include=dev' + rm -rf web/node_modules + npm ci --prefix web --include=dev --no-audit --no-fund + fi + - name: web-prettier + glob: "web/**/*.{css,less,json,js,jsx,ts,tsx}" + root: "web/" + stage_fixed: true + run: | + if [ "${LEFTHOOK_CHECK_ONLY:-}" = "1" ]; then + npx prettier --check --ignore-unknown {staged_files} + else + npx prettier --write --ignore-unknown {staged_files} + fi + - name: web-eslint + glob: "web/**/*.{js,jsx,ts,tsx}" + root: "web/" + stage_fixed: true + run: | + if [ "${LEFTHOOK_CHECK_ONLY:-}" = "1" ]; then + npx eslint {staged_files} + else + npx eslint --fix {staged_files} + fi - name: check-yaml run: python3 tools/hooks/check_files.py yaml - name: check-json diff --git a/test/unit_test/tools/test_check_files.py b/test/unit_test/tools/test_check_files.py new file mode 100644 index 0000000000..22baeebad7 --- /dev/null +++ b/test/unit_test/tools/test_check_files.py @@ -0,0 +1,31 @@ +from pathlib import Path +from subprocess import CompletedProcess + +from tools.hooks import check_files + + +def test_staged_paths_preserve_whitespace(monkeypatch): + paths = b"space name.py\0line\nbreak.go\0" + + def run(command, **kwargs): + assert command == ["git", "diff", "--cached", "--name-only", "--diff-filter=ACMR", "-z"] + assert kwargs == {"check": True, "capture_output": True} + return CompletedProcess(command, 0, stdout=paths) + + monkeypatch.setattr(check_files.subprocess, "run", run) + + assert check_files._staged_paths() == [Path("space name.py"), Path("line\nbreak.go")] + + +def test_case_conflicts_preserve_newlines(monkeypatch, capsys): + paths = b"dir/A\nfile.py\0dir/a\nfile.py\0" + + def run(command, **kwargs): + assert command == ["git", "ls-files", "-z"] + assert kwargs == {"check": True, "capture_output": True} + return CompletedProcess(command, 0, stdout=paths) + + monkeypatch.setattr(check_files.subprocess, "run", run) + + assert check_files.check_case_conflicts([]) == 1 + assert "case conflict:" in capsys.readouterr().err diff --git a/tools/hooks/check_files.py b/tools/hooks/check_files.py index 5cdfbc31ca..ac85253c35 100644 --- a/tools/hooks/check_files.py +++ b/tools/hooks/check_files.py @@ -3,6 +3,7 @@ from __future__ import annotations import ast import json +import os import re import subprocess import sys @@ -23,14 +24,17 @@ def _read_bytes(path: Path) -> bytes: return path.read_bytes() -def _staged_paths() -> list[Path]: +def _git_paths(*args: str) -> list[Path]: proc = subprocess.run( - ["git", "diff", "--cached", "--name-only", "--diff-filter=ACMR"], + ["git", *args, "-z"], check=True, capture_output=True, - text=True, ) - return [Path(line) for line in proc.stdout.splitlines() if line] + return [Path(os.fsdecode(raw_path)) for raw_path in proc.stdout.split(b"\0") if raw_path] + + +def _staged_paths() -> list[Path]: + return _git_paths("diff", "--cached", "--name-only", "--diff-filter=ACMR") def _report(errors: list[str]) -> int: @@ -147,15 +151,10 @@ def check_symlinks(paths: list[Path], fix: bool = False) -> int: def check_case_conflicts(_: list[Path], fix: bool = False) -> int: - proc = subprocess.run( - ["git", "ls-files"], - check=True, - capture_output=True, - text=True, - ) seen: dict[str, str] = {} errors: list[str] = [] - for path in proc.stdout.splitlines(): + for raw_path in _git_paths("ls-files"): + path = str(raw_path) lowered = path.lower() other = seen.get(lowered) if other and other != path: