Pre-commit hooks are fundamentally broken

Source: Hacker News

Starting a New Rust Project

$ mkdir best-fizzbuzz-ever
$ cd best-fizzbuzz-ever
$ cat  main.rs
fn main() { for i in 0.. {
    println ("fizzbuzz");
}}
EOF
$ git init
Initialized empty Git repository in /home/jyn/src/third-website/best-fizzbuzz-ever/.git/
$ git add main.rs
$ git commit --message fizzbuzz
[main (root-commit) 661dc28] fizzbuzz
 1 file changed, 4 insertions(+)
 create mode 100644 main.rs

Neat. Now let’s say I add this to a list of fizz‑buzz projects in different languages, e.g. this one.
They tell me I need to have proper formatting and use consistent style. How rude.

A Pre‑Commit Hook for Formatting

$ cat  pre-commit
#!/bin/sh
set -eu
for f in *.rs; do
  rustfmt --check "$f"
done
EOF
$ chmod +x pre-commit
$ ln -s ../../pre-commit .git/hooks/pre-commit
$ git add pre-commit
$ git commit --message "add pre-commit hook"
Diff in /home/jyn/src/third-website/best-fizzbuzz-ever/src/main.rs:1:
-fn main() { for i in 0.. {
-    println ("fizzbuzz");
-}}
+fn main() {
+    for i in 0.. {
+        println("fizzbuzz");
+    }
+}

Neat! Let’s commit that change.

$ rustfmt main.rs
$ git commit --message "add pre-commit hook"
[main 3be7b87] add pre-commit hook
 1 file changed, 4 insertions(+)
 create mode 100755 pre-commit
$ git status
On branch main
Changes not staged for commit:
  (use "git add ..." to update what will be committed)
  (use "git restore ..." to discard changes in working directory)
	modified:   main.rs

Oops… We fixed the formatting, but we didn’t actually stage the changes.
The pre‑commit hook runs on the working tree, not on the index, so it didn’t catch the issue. We can see that the version tracked by Git still has the wrong formatting:

$ git show HEAD:main.rs
fn main() { for i in 0.. {
    println ("fizzbuzz");
}}

Making the Hook Smarter

Let’s check out all the files in the index into a temporary directory and run the hook there.

$ cat  pre-commit
#!/bin/sh
set -eu

tmpdir=$(mktemp -d --tmpdir "$(basename "$(realpath .)")-pre-commit.XXXX")
trap 'rm -r "$tmpdir"' EXIT
git checkout-index --all --prefix="$tmpdir/"
for f in $tmpdir/*.rs; do
  rustfmt --check "$f"
done
EOF
$ git add pre-commit
$ git commit --message "make pre-commit hook smarter"
Diff in /tmp/best-fizzbuzz-ever-pre-commit.ZNyw/main.rs:1:
-fn main() { for i in 0.. {
-    println ("fizzbuzz");
-}}
+fn main() {
+    for i in 0.. {
+        println("fizzbuzz");
+    }
+}

Yay! That caught the issue.

Now let’s add our Rust program to the collection of fizz‑buzz programs.

$ git add main.rs
$ git commit --message "make pre-commit hook smarter"
[main 3cb40f6] make pre-commit hook smarter
 2 files changed, 11 insertions(+), 4 deletions(-)

$ git remote add upstream https://github.com/joshkunz/fizzbuzz
$ git fetch upstream
remote: Enumerating objects: 222, done.
remote: Total 222 (delta 0), reused 0 (delta 0), pack-reused 222 (from 1)
Receiving objects: 100% (222/222), 29.08 KiB | 29.08 MiB/s, done.
Resolving deltas: 100% (117/117), done.
From https://github.com/joshkunz/fizzbuzz
 * [new branch]      master -> upstream/master

$ git rebase upstream
Successfully rebased and updated refs/heads/main.

A Final Tweak

$ sed -i '1i // Written by jyn' main.rs
$ git commit main.rs --message "mark who wrote fizzbuzz"
Diff in /tmp/best-fizzbuzz-ever-pre-commit.n1Pj/fizzbuzz-traits.rs:4:
 use std::iter;

 struct FizzBuzz {
-    from : i32
-  , to : i32
+    from: i32,
+    to: i32,
 }

 impl FizzBuzz {

Uh‑huh. The code that was already in the repository wasn’t formatted according to rustfmt.
Our script runs on every file in the repo, so it blocks the commit.

Limiting the Hook to Modified Files

$ cat  pre-commit
#!/bin/sh
set -eu

files=$(git diff --name-only --cached --no-ext-diff --diff-filter=d)

tmpdir=$(mktemp -d --tmpdir "$(basename "$(realpath .)")-pre-commit.XXXX")
trap 'rm -r "$tmpdir"' EXIT

printf %s "$files" | tr '\n' '\0' | xargs -0 git checkout-index --prefix="$tmpdir/"
for f in $tmpdir/*.rs; do
  rustfmt --check "$f"
done
EOF
$ git commit main.rs pre-commit \
  --message "update main.rs; make pre-commit even smarter"
[main f2925bc] update main.rs; make pre-commit even smarter
 2 files changed, 5 insertions(+), 1 deletion(-)

Alright. Cool.

Dealing with Rebases and Empty Commits

Let’s simulate an old PR that needs to be rebased onto main.

$ git checkout upstream/HEAD   # Simulate an old PR by checking out an old commit
HEAD is now at 56bf3ab Adds E to the README
$ echo 'fn main() { println!("this counts as fizzbuzz, right?"); }' > print.rs
$ git add print.rs
$ git commit --message "Add print.rs"
[detached HEAD 3d1bbf7] Add print.rs
 1 file changed, 1 insertion(+)
 create mode 100644 print.rs

Now we want to edit the commit message.

$ git rebase -i main   # Rebase this branch onto our main branch
reword 3d1bbf7 Add print.rs
# Rebase f2925bc..3d1bbf7 onto f2925bc (1 command)

The Problem

Error: file `/tmp/best-fizzbuzz-ever-pre-commit.p3az/*.rs` does not exist
Could not apply 3d1bbf7... Add print.rs

Two things went wrong:

1. The pre‑commit hook can’t handle commits that don’t contain any Rust files.
2. The hook ran while we were rebasing, i.e., on a branch that was in the middle of being rewritten.

Fixing the first issue alone isn’t enough because we don’t control other people’s branches. They might:

• Use git commit --no-verify.
• Not have a pre‑commit hook installed at all.
• Have a branch that originally passed the hook but fails after a rebase (e.g., if the hook runs cargo check).

Takeaways

• Run hooks on the index, not the working tree.
• Limit checks to files that actually changed.
• Make hooks robust to empty file sets and rebase operations.
• Consider using server‑side checks (e.g., CI) for a final safety net.

Why pre‑commit Hooks Are Problematic

They might have had a branch that used an old version of the hook that didn’t have as many checks as a later version.
Our only real choice here is to pass --no-verify to git rebase every time we run it, and to git commit for every commit in the rebase we modify, and possibly even to every git merge we run outside of a rebase.

This is because pre‑commit hooks are a fundamentally broken idea:

• Code does not exist in isolation.
• Commits that are local to a developer’s machine never go through CI.
• Commits don’t even necessarily mean that the code is ready to publish—pre‑commit hooks don’t run on git stash for a reason!

I don’t use git stash; I use git commit so that my stashes are tied to a branch, and hooks completely break this workflow.

There are a bunch of other foot‑guns with pre‑commit hooks. This doesn’t even count the fact that nearly all pre‑commit hooks are implemented in a broken way, blindly running on the worktree, and are slow or unreliable—or both.

Don’t get me started on pre‑commit hooks that try to add things to the commit you’re about to make. Please just don’t use them. Use pre‑push instead.¹

pre‑push hooks avoid almost all of these issues.

Tips for Writing a pre‑push Hook

1. Run on the index, not the working tree, as described above.³
2. Only add checks that are fast and reliable.
   • Checks that touch the network should never go in a hook.
   • Checks that are slow and require an up‑to‑date build cache should never go in a hook.
   • Checks that require credentials or a running local service should never go in a hook.
3. Be as quiet as possible. This hook runs buried inside a bunch of other commands, often without the developer knowing that the hook is going to run. Don’t hide important output behind a wall of progress messages.
4. Don’t set the hook up automatically. Whatever tool promises to make this reliable is wrong. There is no way to do this reliably, and the number of times it’s broken on me is more than I can count. Please just add documentation for how to set it up manually, prominently featured in your CONTRIBUTING docs. (You do have contributing docs, right?)

And don’t write pre‑commit hooks!

Additional Observations

• This is really quite slow on large enough repos, but there’s no real alternative. git stash destroys the git index state.
• The only VCS that exposes a FUSE filesystem of its commits is Sapling (see its Eden FS overview), which is poorly supported outside Facebook.
• The best you can do is give up on looking at the whole working copy and only write hooks that read a single file at a time.↩︎⁴

By default this doesn’t happen when running a bare rebase, but the second you add --interactive, nearly anything you do runs a hook. Hooks will also run when you attempt to resolve merge conflicts.↩︎²

For more info about the difference, and a full list of possible hooks, see man 5 githooks.↩︎¹

Notice that I don’t say “only run on changed files”. That’s because it’s not actually possible to reliably determine which branch the current commit is based on (see the discussion here). The best you can do is pick a random branch that looks likely.↩︎³

Footnotes

1. pre‑push hooks nearly avoid all of these issues.
2. Hooks also run when you attempt to resolve merge conflicts.
3. Run on the index, not the working tree, as described above.
4. The only VCS that exposes a FUSE filesystem of its commits is Sapling.