Skip to content

fix(pre-commit): replace pre-commit with shell git hooks and update documentation #43

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
wambitz merged 5 commits into from
Mar 12, 2026
Merged

fix(pre-commit): replace pre-commit with shell git hooks and update documentation #43

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
3930443
fix: replace pre-commit with plain shell git hooks
wambitz Mar 11, 2026
564462f
docs: update README and devcontainer for new git hooks setup
wambitz Mar 11, 2026
596b455
fix: skip clang-tidy in pre-push hook when build dir is missing
wambitz Mar 12, 2026
e25dfb4
refactor: single Docker invocation for pre-push hook
wambitz Mar 12, 2026
9988003
fix: drain stdin in pre-push hook and fix install-hooks comment
wambitz Mar 12, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .devcontainer/devcontainer.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@
}
},

"postCreateCommand": "pre-commit install --hook-type pre-push",
"postCreateCommand": "./scripts/install-hooks.sh",
"postStartCommand": "bash",

// Clean container name and setup
Expand Down
23 changes: 23 additions & 0 deletions .githooks/pre-push
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
#!/usr/bin/env bash
set -e

###############################################################################
# Git pre-push hook
#
# Runs formatting and linting checks before pushing. Delegates to the project
# scripts, which auto-delegate to Docker when run outside the container.
###############################################################################

REPO_ROOT="$(git rev-parse --show-toplevel)"

echo "=== Pre-push: checking formatting ==="
"$REPO_ROOT/scripts/format.sh" --check

Copy link

Copilot AI Mar 11, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The hook runs scripts/lint.sh unconditionally. lint.sh relies on an existing build directory with compile_commands.json (via -p "$BUILD_DIR"), so this hook will fail on fresh clones / before a first CMake configure. Consider adding a preflight check for build/compile_commands.json and emitting a clear instruction (e.g., run cmake -S . -B build) before exiting non-zero (or explicitly skipping lint).

Suggested change
REPO_ROOT="$(git rev-parse --show-toplevel)"
echo "=== Pre-push: checking formatting ==="
"$REPO_ROOT/scripts/format.sh" --check
REPO_ROOT="$(git rev-parse --show-toplevel)"
BUILD_DIR="$REPO_ROOT/build"
echo "=== Pre-push: checking formatting ==="
"$REPO_ROOT/scripts/format.sh" --check
# Ensure clang-tidy prerequisites exist before running lint.
if [ ! -f "$BUILD_DIR/compile_commands.json" ]; then
echo "=== Pre-push: skipping clang-tidy ==="
echo "No compile_commands.json found at '$BUILD_DIR/compile_commands.json'."
echo "Run: cmake -S \"$REPO_ROOT\" -B \"$BUILD_DIR\""
exit 1
fi

Copilot uses AI. Check for mistakes.
BUILD_DIR="$REPO_ROOT/build"
if [ ! -f "$BUILD_DIR/compile_commands.json" ]; then
echo "=== Pre-push: skipping clang-tidy (no compile_commands.json) ==="
echo "Run ./scripts/build.sh first to enable lint checks."
else
echo "=== Pre-push: running clang-tidy ==="
"$REPO_ROOT/scripts/lint.sh"
Copy link

Copilot AI Mar 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On hosts, this hook will typically start Docker twice (once via format.sh delegation and again via lint.sh delegation). Since scripts/docker/exec.sh uses docker run --rm each time, this adds noticeable overhead to every git push. Consider delegating once at the hook level (source scripts/env.sh + scripts/docker/exec.sh and call delegate_to_container at the top) and then run both checks inside that single container invocation.

Copilot uses AI. Check for mistakes.
fi
19 changes: 0 additions & 19 deletions .pre-commit-config.yaml
View file
Open in desktop

This file was deleted.

2 changes: 1 addition & 1 deletion CLAUDE.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ This file provides guidance to Claude Code (claude.ai/code) when working with co
./scripts/docs.sh # Generate Doxygen documentation
```

Pre-commit hooks run on **pre-push** (not pre-commit): `pre-commit install --hook-type pre-push`
Git hooks live in `.githooks/` (version-controlled). Install once after cloning: `./scripts/install-hooks.sh`

All scripts auto-delegate to Docker when run outside the container (`docker run --rm`). The delegation logic lives in `scripts/docker/exec.sh`, sourced by each script via `scripts/env.sh`. Inside the container or CI (`CI=true`), scripts run directly with no overhead. See `docs/ci-container-delegation.md` for details on the CI strategy and a GHCR upgrade path.
Copy link

Copilot AI Mar 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This section mentions that all scripts auto-delegate to Docker when run outside the container, but scripts/install-hooks.sh currently runs directly on the host (no delegate_to_container). Please either adjust the wording to exclude hook installation, or change install-hooks.sh to conform so the guidance here matches actual behavior.

Suggested change
Git hooks live in `.githooks/` (version-controlled). Install once after cloning: `./scripts/install-hooks.sh`
All scripts auto-delegate to Docker when run outside the container (`docker run --rm`). The delegation logic lives in `scripts/docker/exec.sh`, sourced by each script via `scripts/env.sh`. Inside the container or CI (`CI=true`), scripts run directly with no overhead. See `docs/ci-container-delegation.md` for details on the CI strategy and a GHCR upgrade path.
Git hooks live in `.githooks/` (version-controlled). Install once after cloning: `./scripts/install-hooks.sh` (runs directly on the host).
All build/test/utility scripts above auto-delegate to Docker when run outside the container (`docker run --rm`). The delegation logic lives in `scripts/docker/exec.sh`, sourced by each script via `scripts/env.sh`. Inside the container or CI (`CI=true`), scripts run directly with no overhead. See `docs/ci-container-delegation.md` for details on the CI strategy and a GHCR upgrade path.

Copilot uses AI. Check for mistakes.

Expand Down
5 changes: 0 additions & 5 deletions Dockerfile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,6 @@ RUN apt-get update && apt-get install -y \
lcov \
gdb \
valgrind \
python3-pip \
ca-certificates \
tree \
&& rm -rf /var/lib/apt/lists/*
Expand Down Expand Up @@ -70,10 +69,6 @@ RUN echo 'ubuntu ALL=(ALL) NOPASSWD:ALL' > /etc/sudoers.d/ubuntu \
# ------------------------------------------------------------------------------
# RUN sed -i 's/^#force_color_prompt=yes/force_color_prompt=yes/' /home/ubuntu/.bashrc

# -----------------------------------------------------------------------------
# Install pre-commit for code quality checks
# -----------------------------------------------------------------------------
RUN pip3 install --break-system-packages pre-commit cmake-format

# ------------------------------------------------------------------------------
# Copy entrypoint script
Expand Down
Loading
Loading