andrew/libakgl
1
0
Fork 0
Code Issues 69 Pull Requests Actions Packages Projects Releases Wiki Activity

scripts/hooks/ has a pre-commit hook but no pre-push hook #82

New Issue
Closed
opened 2026-08-05 15:53:20 -04:00 by tachikoma · 4 comments
tachikoma commented 2026-08-05 15:53:20 -04:00
Collaborator
Copy Link

This repository has a hooks directory -- scripts/hooks/, installed with
git config core.hooksPath scripts/hooks -- but the only hook in it is pre-commit, which
reindents staged C sources. There is no pre-push hook, so every real gate this repository
has (build with -Werror, ctest, doxygen, the benchmark budgets, valgrind, mutation) runs
only after a push has already reached the forge.

Filed separately from the cppcheck work because it is a gap in its own right, and because
the cppcheck issue's "wire it into the pre-push hook" step has nowhere to go until this
exists.

Why it is cheap to build here

Cheaper than in akbasic or libakerror, which have no hook infrastructure at all: the
directory exists, core.hooksPath scripts/hooks is already the documented install line,
and anyone with the pre-commit hook working gets a pre-push hook the moment it lands, with
no second instruction to follow.

libakstdlib already solved the hard part. .githooks/pre-push there runs the default build
with -Werror plus ctest, the ASan/UBSan build plus ctest, and doxygen -- a few seconds all
in -- and puts the slow mutation harness behind AKSL_HOOK_MUTATION=1. It is worth reading
before writing this one; most of the decisions are already made and commented there:

  • Builds go under .git/, so the hook never disturbs the developer's own build/.
  • Each step runs quietly and dumps its log only on failure.
  • A push that only deletes refs, or has nothing to push, exits 0 immediately.
  • An optional tool that is missing is skipped with a warning, not failed on. A hook that
    fails closed on a missing optional tool only teaches everyone to pass --no-verify.
  • git push --no-verify remains the escape hatch, and the hook says so when it aborts.

scripts/hooks/pre-commit is the other model to follow, for the local conventions: sh
rather than bash, set -eu, leave merge resolution alone, and warn-and-skip rather than
fail when an optional tool is absent.

What to build

  • scripts/hooks/pre-push, alongside the existing pre-commit.
  • Gates it should run, cheapest first: scripts/cppcheck.sh (2 s measured, once the
    cppcheck issue lands), then the AKGL_WERROR=ON build plus ctest.
  • scripts/memcheck.sh and the mutation harness go behind environment variables --
    valgrind over every suite is not a per-push cost.
  • The perf suites should be excluded or scaled down, not run. tests/benchutil.h
    already refuses to enforce budgets in an unoptimized build, and a budget measured on a
    developer's busy laptop is not a measurement.
  • Note in README.md/AGENTS.md that the existing core.hooksPath scripts/hooks line now
    installs two hooks, not one.

Known limit, worth stating up front

core.hooksPath is per-clone local configuration. Git cannot install a hook for you, so
this gate is advisory by construction and CI stays the hard gate. That is the accepted
tradeoff, not a defect in the design -- the hook exists to make the failure cheap, not to
make it impossible.

This repository has a hooks directory -- `scripts/hooks/`, installed with `git config core.hooksPath scripts/hooks` -- but the only hook in it is `pre-commit`, which reindents staged C sources. There is no pre-push hook, so every real gate this repository has (build with `-Werror`, ctest, doxygen, the benchmark budgets, valgrind, mutation) runs only after a push has already reached the forge. Filed separately from the cppcheck work because it is a gap in its own right, and because the cppcheck issue's "wire it into the pre-push hook" step has nowhere to go until this exists. ## Why it is cheap to build here Cheaper than in akbasic or libakerror, which have no hook infrastructure at all: the directory exists, `core.hooksPath scripts/hooks` is already the documented install line, and anyone with the pre-commit hook working gets a pre-push hook the moment it lands, with no second instruction to follow. libakstdlib already solved the hard part. `.githooks/pre-push` there runs the default build with `-Werror` plus ctest, the ASan/UBSan build plus ctest, and doxygen -- a few seconds all in -- and puts the slow mutation harness behind `AKSL_HOOK_MUTATION=1`. It is worth reading before writing this one; most of the decisions are already made and commented there: - Builds go under `.git/`, so the hook never disturbs the developer's own `build/`. - Each step runs quietly and dumps its log only on failure. - A push that only deletes refs, or has nothing to push, exits 0 immediately. - An optional tool that is missing is skipped with a warning, not failed on. A hook that fails closed on a missing optional tool only teaches everyone to pass `--no-verify`. - `git push --no-verify` remains the escape hatch, and the hook says so when it aborts. `scripts/hooks/pre-commit` is the other model to follow, for the local conventions: `sh` rather than bash, `set -eu`, leave merge resolution alone, and warn-and-skip rather than fail when an optional tool is absent. ## What to build - [ ] `scripts/hooks/pre-push`, alongside the existing `pre-commit`. - [ ] Gates it should run, cheapest first: `scripts/cppcheck.sh` (2 s measured, once the cppcheck issue lands), then the `AKGL_WERROR=ON` build plus ctest. - [ ] `scripts/memcheck.sh` and the mutation harness go behind environment variables -- valgrind over every suite is not a per-push cost. - [ ] The perf suites should be excluded or scaled down, not run. `tests/benchutil.h` already refuses to enforce budgets in an unoptimized build, and a budget measured on a developer's busy laptop is not a measurement. - [ ] Note in README.md/AGENTS.md that the existing `core.hooksPath scripts/hooks` line now installs two hooks, not one. ## Known limit, worth stating up front `core.hooksPath` is per-clone local configuration. Git cannot install a hook for you, so this gate is advisory by construction and CI stays the hard gate. That is the accepted tradeoff, not a defect in the design -- the hook exists to make the failure cheap, not to make it impossible.
tachikoma added the test-coverageblast-radius:mediumstatus::ready labels 2026-08-05 15:53:21 -04:00
logikoma was assigned by andrew 2026-08-05 15:55:43 -04:00
logikoma commented 2026-08-05 16:51:13 -04:00
Collaborator
Copy Link

Beginning work on issue #82: I will add and validate the missing pre-push hook.

Beginning work on issue #82: I will add and validate the missing pre-push hook.
logikoma commented 2026-08-05 18:32:00 -04:00
Collaborator
Copy Link

Beginning work on the pre-push hook and installation documentation.

Beginning work on the pre-push hook and installation documentation.
logikoma commented 2026-08-05 18:34:04 -04:00
Collaborator
Copy Link

Implementation is complete. The synthetic pre-push run reached the real configure step but cannot proceed in this environment because SDL3 development files are unavailable; shell syntax and diff checks pass.

Implementation is complete. The synthetic pre-push run reached the real configure step but cannot proceed in this environment because SDL3 development files are unavailable; shell syntax and diff checks pass.
logikoma added status::review and removed status::ready labels 2026-08-05 18:35:46 -04:00
logikoma commented 2026-08-05 18:35:55 -04:00
Collaborator
Copy Link

Work is done. Pull request #84 is open, requests Andrew's review, and the ticket is labeled status::review: #84

Work is done. Pull request #84 is open, requests Andrew's review, and the ticket is labeled status::review: https://source.starfort.tech/andrew/libakgl/pulls/84
Sign in to join this conversation.
No Branch/Tag Specified
Branches Tags
Labels
Clear labels
abi-break
Cannot land without a soname bump
 api-gap
A caller cannot express something
 blast-radius:high
Ordering: highest blast radius
 blast-radius:low
Ordering: lowest blast radius
 blast-radius:medium
Ordering: middling blast radius
 defect
Wrong behaviour reachable from the public API
 design-decision
Needs a decision written down before code
 docs
Documentation, comments, AGENTS.md
 hygiene
Style, dead code, consistency
 packaging
Build, install, pkg-config, CMake config
 performance
Has a measurement behind it
 status::blocked
Waiting on something else -- a decision, a dependency, or another issue
 status::grooming
Not yet worked out: scope, approach or priority still being settled
 status::ready
Understood well enough to start: scope and approach are settled
 status::review
A pull request has been generated and this ticket is waiting on review of that PR
 test-coverage
A gap in the suites, not a defect
No Label blast-radius:medium status::review test-coverage
Milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
logikoma tachikoma andrew
No Assignees logikoma
2 Participants
Notifications
Due Date
No due date set.
Blocks
#83 Gate every push on cppcheck: 103 findings, and two files cppcheck cannot parse at all
andrew/libakgl
Reference: andrew/libakgl#82
Delete Branch "%!s()"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?