diff --git a/.githooks/pre-commit b/.githooks/pre-commit index 43d5af1..35c929d 100755 --- a/.githooks/pre-commit +++ b/.githooks/pre-commit @@ -17,12 +17,21 @@ if [ -n "$staged_scratch_files" ]; then done fi -# 1. Update templates and sort pubspec if it was staged or templates changed -echo "๐Ÿ“ฆ Syncing templates to pubspec.yaml..." -dart run tools/update_templates.dart +staged_templates=$(git diff --cached --name-only | grep '^assets/templates/') + +# 1. Sync the template folders into pubspec.yaml's assets, when they changed. +# +# Only then: it is a list of folders, and nothing else can change it. It used +# to run on every commit, and `dart run` builds the dependency graph's native +# hooks first whenever they are stale โ€” after any branch switch or dependency +# change โ€” which made it ~45 s of a commit that touched no template. +if [ -n "$staged_templates" ]; then + echo "๐Ÿ“ฆ Syncing templates to pubspec.yaml..." + dart run tools/update_templates.dart +fi # 2. Sort pubspec if it was staged -if [ -n "$staged_pubspec" ] || git diff --cached --name-only | grep -q 'assets/templates/'; then +if [ -n "$staged_pubspec" ] || [ -n "$staged_templates" ]; then echo "๐Ÿ“ฆ Sorting pubspec.yaml dependencies..." dart run sort_pubspec_dependencies git add pubspec.yaml @@ -55,52 +64,15 @@ if [ -n "$staged_dart_files" ]; then done fi -# 4. Analyze, when the commit could actually affect analysis -# -# This was commented out, which meant a broken commit was accepted and the -# breakage surfaced at the next `git push` โ€” several commits later, blamed on -# whoever happened to run into it. Both of the last two push failures were -# that. -# -# `dart analyze`, not `flutter analyze`. The note that used to sit here had it -# backwards: it said `flutter analyze` was used *because* the Flutter tool sets -# up the analyzer plugins this repo's analysis_options registers. It does not. -# `flutter analyze` never loads them, which is why 34 riverpod_lint findings -# accumulated while every hook and CI run reported a clean tree โ€” they were -# only ever visible in the IDE. `dart analyze` does load them. +# No analysis here. `dart analyze --fatal-infos` takes ~40-55 s on this +# project even for a single file โ€” most of it loading the analyzer plugins โ€” +# which is a commit you learn to skip with --no-verify. The IDE shows the same +# findings as you type, and pre-push analyzes the whole tree before anything +# reaches the remote, so a broken commit still cannot leave your machine. # -# `--fatal-infos` because the two disagree on what counts as failure: -# `flutter analyze` exits non-zero on an info, plain `dart analyze` exits 0. -# Without the flag this swap would have *loosened* the gate โ€” and most of what -# riverpod_lint reports (`avoid_public_notifier_properties`, and every lint in -# `all_lints` that is info-level) would have stopped blocking anything. -# -# Costs about 11s more than `flutter analyze` did, which is the plugin loading. -# -# Skipped for commits that cannot change the result (docs, assets, workflows) -# so that writing a README stays instant. pre-push analyzes unconditionally, -# so nothing reaches the remote unchecked either way. -staged_analysis_config=$(git diff --cached --name-only --diff-filter=ACM | grep -E '(^|/)analysis_options\.yaml$') - -if [ -n "$staged_dart_files" ] || [ -n "$staged_pubspec" ] || [ -n "$staged_analysis_config" ]; then - echo "๐Ÿ” Analyzing..." - if ! dart analyze --fatal-infos; then - # Generated *.g.dart files are gitignored, so a fresh clone or a branch - # switch can leave the tree needing a regenerate. Say so plainly rather - # than leaving a wall of undefined-symbol errors that all have the same - # one-line cause. - echo "" - echo "โŒ Analysis failed. If these are missing generated symbols, run:" - echo " dart run build_runner build --delete-conflicting-outputs" - echo "Commit blocked; use --no-verify to override." - exit 1 - fi -fi - -# Tests deliberately stay in pre-push. They take ~30s against ~430 tests, -# which is tolerable once per push but not once per commit โ€” and a commit is -# not the thing that reaches anyone else. Run them yourself before -# committing; these hooks are the backstop, not the gate. +# `dart analyze`, never `flutter analyze`, wherever it runs: `flutter analyze` +# does not load the analyzer plugins analysis_options.yaml registers, which is +# how 34 riverpod_lint findings once accumulated behind a clean hook. echo "โœ… All pre-commit checks passed!" exit 0 diff --git a/.githooks/pre-push b/.githooks/pre-push index 9816e9c..79ee0a3 100755 --- a/.githooks/pre-push +++ b/.githooks/pre-push @@ -1,15 +1,27 @@ #!/bin/sh -# Runs the gates CI runs, before the push rather than after it. +# Runs the gates CI runs, before the push rather than after it โ€” but only the +# ones the pushed commits can affect. # -# This is deliberately NOT in pre-commit. Analysis and the suite take about a -# minute together, which is fine once per push and not fine on every commit โ€” -# a hook slow enough to be worth skipping gets skipped, and then it protects -# nothing. Committing stays instant; pushing is where the cost belongs, -# because that is the point after which a red result costs someone else time. +# This is deliberately NOT in pre-commit: analysis alone is ~40-55 s here, and a +# hook slow enough to be worth skipping gets skipped, and then it protects +# nothing. Pushing is where the cost belongs, because that is the point after +# which a red result costs someone else time. # -# Mirrors the analyze-and-test job in .github/workflows/ci.yml. Formatting and -# import sorting are pre-commit's job and are not repeated here. +# It mirrors CI's analyze, test and package jobs, which it used to run in full +# on every push โ€” ~2.5 min warm on an M-series Mac, docs included. Now: +# +# - a push that touches nothing they check (docs, workflows) runs none; +# - the packages are checked only when something under packages/ changed: +# they cannot import the app, so app code cannot break them. An app-only +# push went from ~155 s to ~95 s. +# +# One thing at a time, deliberately. Measured on an 8-core Mac: analysis +# alongside the tests took 135 s against 96 s in turn, and the eleven packages +# all at once 107 s against 86 s in turn (three at a time saved 9 s). Each +# already spreads over every core, so running them together only fights. +# +# Formatting and import sorting are pre-commit's job and are not repeated here. # # `dart analyze --fatal-infos`, not `flutter analyze`: the latter does not load # the analyzer plugins analysis_options.yaml registers, so it reports no @@ -29,19 +41,52 @@ # below runs in the working tree it was invoked from. unset GIT_DIR GIT_WORK_TREE GIT_INDEX_FILE -# Deleting a remote branch pushes no code, so there is nothing to check. -# stdin carries " " per ref; -# an all-zero local sha is a delete. +zero=0000000000000000000000000000000000000000 + +# What the push changes, across every ref in it. stdin carries +# " " per ref. A deleted ref +# (all-zero local sha) pushes no code. A new branch is compared with where it +# left main. If no base can be found, everything is checked: a wasted minute +# is cheaper than an unchecked push. pushing_code=0 -while read -r _ local_sha _ _; do +check_everything=0 +changed="" +while read -r _ local_sha _ remote_sha; do case "$local_sha" in - 0000000000000000000000000000000000000000) ;; - '') ;; - *) pushing_code=1 ;; + "$zero" | '') continue ;; esac + pushing_code=1 + if [ "$remote_sha" = "$zero" ]; then + base=$(git merge-base "$local_sha" origin/main 2>/dev/null || true) + else + base=$remote_sha + fi + if [ -n "$base" ] && git cat-file -e "$base^{commit}" 2>/dev/null; then + changed="$changed +$(git diff --name-only "$base" "$local_sha")" + else + check_everything=1 + fi done [ "$pushing_code" -eq 1 ] || exit 0 +touches() { + [ "$check_everything" -eq 1 ] || printf '%s\n' "$changed" | grep -qE "$1" +} + +# What can change the app's analysis or tests: its code, its tests, the +# packages and assets it builds from, its dependencies and its lint setup. +app_paths='^(lib/|test/|packages/|assets/|tools/|pubspec\.(yaml|lock)$|analysis_options\.yaml$|build\.yaml$|dart_test\.yaml$)' +check_app=0 +check_packages=0 +touches "$app_paths" && check_app=1 +touches '^packages/' && check_packages=1 + +if [ "$check_app" -eq 0 ] && [ "$check_packages" -eq 0 ]; then + echo "โœ… Nothing in this push changes what the checks cover โ€” skipping them." + exit 0 +fi + # A fresh clone or worktree has no dependencies or generated code yet, which # would fail every check below for reasons that have nothing to do with the # push. Set up whatever is missing first; on a ready checkout this is quick. @@ -50,56 +95,55 @@ if ! tools/bootstrap.sh; then exit 1 fi -echo "๐Ÿ” Analyzing..." -if ! dart analyze --fatal-infos; then - # Generated *.g.dart files are gitignored, so a fresh clone or a branch - # switch can leave the tree needing a regenerate. Say so plainly rather - # than leaving a wall of undefined-symbol errors that all have the same - # one-line cause. - echo "" - echo "โŒ Analysis failed. If these are missing generated symbols, run:" - echo " dart run build_runner build --delete-conflicting-outputs" - exit 1 -fi +if [ "$check_app" -eq 1 ]; then + echo "๐Ÿ” Analyzing..." + if ! dart analyze --fatal-infos; then + # Generated *.g.dart files are gitignored, so a fresh clone or a branch + # switch can leave the tree needing a regenerate. Say so plainly rather + # than leaving a wall of undefined-symbol errors that all have the same + # one-line cause. + echo "" + echo "โŒ Analysis failed. If these are missing generated symbols, run:" + echo " dart run build_runner build --delete-conflicting-outputs" + exit 1 + fi -# The 'arduino' tag marks tests that shell out to a local arduino-cli -# toolchain. CI excludes them and so does this โ€” run them yourself if you -# touched the compile path. -echo "๐Ÿงช Running tests..." -if ! flutter test --exclude-tags arduino; then - echo "" - echo "โŒ Tests failed. Push blocked; use --no-verify to override." - exit 1 + # The 'arduino' tag marks tests that shell out to a local arduino-cli + # toolchain. CI excludes them and so does this โ€” run them yourself if you + # touched the compile path. + echo "๐Ÿงช Running tests..." + if ! flutter test --exclude-tags arduino; then + echo "" + echo "โŒ Tests failed. Push blocked; use --no-verify to override." + exit 1 + fi fi # Packages are separate pub roots: the root analyze excludes them and the root # test run does not descend into them, so without this they reach CI unchecked. -for pkg in packages/*/; do - [ -f "$pkg/pubspec.yaml" ] || continue - echo "๐Ÿ“ฆ Checking $pkg" - # No package registers analyzer plugins โ€” only the root does โ€” so here the - # difference from `flutter analyze` is purely `--fatal-infos`. Kept - # identical to the root command anyway, so there is one analyze incantation - # in this repo rather than two that drift. - if ! (cd "$pkg" && dart analyze --fatal-infos); then - echo "" - echo "โŒ $pkg failed analysis. Push blocked; use --no-verify to override." - exit 1 - fi - # Not every package is a library with a suite โ€” composite_plugin is a - # generated analyzer plugin. `flutter test` exits 1 on "Test directory not - # found", which is not a test failure, so check before running rather than - # treating a testless package as broken. Said out loud so a package that - # *should* have tests and lost them is visible rather than silently passing. - if [ -d "$pkg/test" ]; then - if ! (cd "$pkg" && flutter test); then +if [ "$check_packages" -eq 1 ]; then + for pkg in packages/*/; do + [ -f "$pkg/pubspec.yaml" ] || continue + echo "๐Ÿ“ฆ Checking $pkg" + if ! (cd "$pkg" && dart analyze --fatal-infos); then echo "" - echo "โŒ $pkg failed its tests. Push blocked; use --no-verify to override." + echo "โŒ $pkg failed analysis. Push blocked; use --no-verify to override." exit 1 fi - else - echo " (no test/ directory โ€” analysis only)" - fi -done + # Not every package has a suite โ€” composite_plugin is a generated + # analyzer plugin. `flutter test` exits 1 on "Test directory not + # found", which is not a test failure, so check before running. Said + # out loud so a package that *should* have tests and lost them shows. + if [ -d "$pkg/test" ]; then + if ! (cd "$pkg" && flutter test); then + echo "" + echo "โŒ $pkg failed its tests. Push blocked; use --no-verify to override." + exit 1 + fi + else + echo " (no test/ directory โ€” analysis only)" + fi + done +fi echo "โœ… Ready to push." diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index f1cfc67..d91f1c7 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -58,8 +58,10 @@ you touch the compile path. CI runs these on every pull request; running them first saves a round trip. `./setup_hooks.sh`, once after cloning, has git do it for you: the pre-commit -hook formats, sorts imports and analyzes what you staged, and the pre-push hook -runs the analysis and the tests below. +hook formats and sorts imports in what you staged (analysis is left to your IDE, +since it takes the better part of a minute here), and the pre-push hook runs the +analysis and the tests below โ€” those the push can affect, so a docs change runs +none of them. ```bash dart analyze --fatal-infos # must be clean, infos included โ€” as CI runs it @@ -72,8 +74,9 @@ for p in packages/*/; do [ -d "$p/test" ] && (cd "$p" && flutter test); done Packages under `packages/` carry their own `pubspec.yaml` and `analysis_options.yaml` and are checked on their own terms โ€” the root `flutter analyze` excludes them and the root `flutter test` does not descend -into them. CI loops over every package, and so does the pre-push hook: all of -them are analyzed, and the ones with a `test/` directory are tested. +into them. CI checks every package, and the pre-push hook does too whenever a +push changes something under `packages/`: all of them are analyzed, and the ones +with a `test/` directory are tested. (`composite_plugin` is a generated analyzer plugin and has none.) `flutter analyze` exits non-zero on **warnings and infos**, not just errors, so diff --git a/setup_hooks.sh b/setup_hooks.sh index d8ae443..0602689 100755 --- a/setup_hooks.sh +++ b/setup_hooks.sh @@ -6,10 +6,13 @@ # ./setup_hooks.sh # # What you get: -# pre-commit โ€” formats staged Dart, sorts imports, syncs templates into -# pubspec.yaml, sorts dependencies. Fast; runs on every commit. -# pre-push โ€” flutter analyze + flutter test, the same gates CI runs. -# About a minute, which is why it is not on commit. +# pre-commit โ€” formats staged Dart and sorts its imports; syncs templates +# into pubspec.yaml and sorts dependencies when those changed. +# A few seconds; runs on every commit. No analysis โ€” that is +# the IDE's and pre-push's. +# pre-push โ€” `dart analyze` and the tests, the same gates CI runs, for +# what the push changes: none for docs, the packages only when +# packages/ changed. # # Both live in .githooks/, which is committed โ€” unlike .git/hooks/, which is # why `git config core.hooksPath` exists at all.