Skip to content

fix(hooks): scope pre-commit clang-tidy to staged changes (#1264) - #1310

Open
Yyunozor wants to merge 1 commit into
DeusData:mainfrom
Yyunozor:fix/1264-hook-diff-aware-clang-tidy
Open

fix(hooks): scope pre-commit clang-tidy to staged changes (#1264)#1310
Yyunozor wants to merge 1 commit into
DeusData:mainfrom
Yyunozor:fix/1264-hook-diff-aware-clang-tidy

Conversation

@Yyunozor

Copy link
Copy Markdown

Fixes #1264

Problem

scripts/hooks/pre-commit runs make -j3 -f Makefile.cbm lint
unconditionally on every commit. lint includes lint-tidy, which runs
clang-tidy over the whole LINT_SRCS tree. On a clean main checkout
that surfaces ~5,100 pre-existing error:-level findings across 113
files (mostly readability-magic-numbers), unrelated to whatever the
commit touches. The hook blocks every commit for every contributor with
clang-tidy on PATH — reproduced on a clean checkout, one-line non-C
change.

Fix

Add lint-tidy-diff (scripts/lint-tidy-diff.sh), a Makefile target
running the same clang-tidy binary/config, but passing clang-tidy's own
-line-filter so a diagnostic can only fire on lines the commit actually
added or modified (parsed from git diff --cached -U0 hunk headers).
Pre-existing findings on untouched lines are never reported, whether in
an untouched file or an untouched line of a touched file — baseline-aware
at the line level, not just the file level.

The hook now runs lint-ci (unchanged: cppcheck + clang-format +
no-suppress, already CI's contract) plus lint-tidy-diff instead of the
full lint target. make lint, make lint-tidy, and scripts/lint.sh
are untouched, still the full-tree audit; CI (_lint.yml
scripts/lint.sh --ci) never ran clang-tidy and is unaffected. This
follows the scoping the issue itself suggested (remediation option 2), at
line rather than file granularity.

Verification

  • Reproduced: clean checkout, git commit -s on a one-line
    CONTRIBUTING.md change, clang-tidy on PATH → blocked, 5,190
    pre-existing findings, commit did not happen.
  • After the fix, same clang-tidy binary/config:
    • No staged .c changes → skipped, exit 0.
    • Clean addition to internal/cbm/cbm.c (one of the dirtiest files, 57
      pre-existing findings on a full-file run of the same edit) → exit 0,
      nothing reported.
    • A deliberate new finding (return 424242;) added to the same file →
      exit 2, reports only that one finding.
    • Header-only change (no paired .c change) → skipped, exit 0 (see
      Known limitation).
  • scripts/lint.sh (cppcheck + clang-format + no-suppress) unaffected,
    still clean.

Known limitation

clang-tidy reports a header's diagnostics under whatever path it was
#include-d with, not reliably its working-tree path (verified:
absolute vs. search-path-relative, depending on include style). So
lint-tidy-diff scopes .c files only; a header-only commit isn't
covered and needs make lint-tidy. A proper diff-to-header mapping would
be a natural follow-up, left for the maintainer to size.

@Yyunozor
Yyunozor requested a review from DeusData as a code owner July 28, 2026 00:01
)

make -j3 -f Makefile.cbm lint runs the full lint-tidy sweep, which
currently surfaces ~5,100 pre-existing clang-tidy findings across 113
files. Since the pre-commit hook (scripts/hooks/pre-commit) runs that
target unconditionally, it blocks every commit for every contributor
who has clang-tidy on PATH, regardless of what the commit touches.

Add lint-tidy-diff (scripts/lint-tidy-diff.sh), which runs the same
clang-tidy binary/config but passes clang-tidy's own -line-filter so
only lines the commit actually added or modified can produce a
diagnostic. Pre-existing findings on untouched lines are not reported,
whether they are in an untouched file or on an untouched line of a
file the commit does touch. The hook now runs lint-ci (unchanged:
cppcheck + clang-format + no-suppress) plus lint-tidy-diff instead of
the full lint target; make lint / make lint-tidy / scripts/lint.sh are
unchanged and remain the full-tree audit.

Signed-off-by: Yyunozor <yyunozor@icloud.com>
@Yyunozor
Yyunozor force-pushed the fix/1264-hook-diff-aware-clang-tidy branch from 4590016 to ca796df Compare July 28, 2026 16:33
@DeusData DeusData added bug Something isn't working ux/behavior Display bugs, docs, adoption UX priority/normal Standard review queue; useful PR with ordinary maintainer urgency. labels Jul 28, 2026
@DeusData DeusData added this to the 0.9.1-rc milestone Jul 28, 2026
@DeusData

DeusData commented Jul 28, 2026

Copy link
Copy Markdown
Owner

Thank you for the contribution and for reproducing how repository-wide clang-tidy findings block unrelated staged changes. This is now triaged as a normal-priority contributor-workflow bug for 0.9.1-rc. Our community PR queue is currently quite full, so it may take a little time before we can complete the review and, if approved, merge it. We are doing our best to support community contributions and will return with code-grounded feedback as capacity opens.

@DeusData

Copy link
Copy Markdown
Owner

A quick note so this does not sit here looking like your problem: neither of your red checks is caused by your change.

Your diff touches Makefile.cbm and two lint-hook scripts. The failures are:

  1. the Windows PATH smoke seam (FAIL: invalid Windows PATH smoke seam fell back to the live registry) — a main-side issue fixed by fix(main): guard the seam call sites instead of stubbing the seams #1346 after your run; and
  2. an AddressSanitizer access-violation in cbm_daemon_ipc_connection_interrupt (src/daemon/ipc.c:5608) during test_daemon_ipc_wait_forever_is_interruptible, which crashed the suite at pass=0.

A lint-hook and Makefile change cannot cause a daemon-IPC ASan crash. That second one is a production crash in our own code and I have flagged it internally for its own investigation — thank you for surfacing it, entirely by accident.

So nothing is needed from you on the CI front; a rebase would pick up the smoke fix, but the ASan crash is ours. Your PR is queued for a proper content review.

Apologies for the delay — we are working through a large backlog oldest-first, and I did not want a crash of ours sitting on your PR looking like your bug.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working priority/normal Standard review queue; useful PR with ordinary maintainer urgency. ux/behavior Display bugs, docs, adoption UX

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Local pre-commit hook (scripts/hooks/pre-commit -> make lint) blocks every commit: ~5,100 pre-existing clang-tidy findings across 113 files

2 participants