Fix install-hooks.bash script

[sellout]

Overview

I hadn’t realized there were Git hooks, but noticed them when I was adding the Weeder script.

So, I documented that they exist, and then fixed the script since it wasn’t working for me.

It now works

• when run it from anywhere in the repo (not just from two directories down somewhere),
• when core.hooksPath is set (i.e., $GIT_DIR/hooks isn’t the right place),
• when run from a worktree (which doesn’t necessarily have its own hooks dir),
• when the hooks directory doesn’t exist, and
• when the hooks already exist (if you pass -f).

Implementation notes

This also replaces ln -s with cp so that in case the particular worktree it was run from is renamed or deleted, it doesn’t break the hooks for the repository.

Interesting/controversial decisions

I didn’t change this, but I don’t think a pre-commit hook should check that tests pass (or even that compilation succeeds). It’s good practice to commit failing tests ahead of the fix, for example. Or to rearrange code in one commit, and then fix the compilation errors in the next.

However, passing --no-verify to a git command will bypass its hooks, so it’s still easy enough to do those things with these hooks in place.

Having tests be part of pre-push makes more sense, because you may be pushing to an open PR and don’t want to have to force push or add a new commit when you realize you just pushed broken code. And it’s still useful to push broken tests (so you can see CI failures), but the “publishing” nature of push means you’d want to be more careful about it.

[aryairani]

Yeah I forgot about these too, I don't use them currently.

I agree we shouldn't have to pass tests to commit; I'm not really sure what the right check is.
./scripts/check.sh is what I run when I'm trying to be careful; maybe that?

Another concern is that check.sh, test.sh, or any stack build command can change the build settings in a way that invalidates your build cache and thus slows down your next development build with your original settings.

The failure message should probably tell you how to bypass with --no-verify (unless git already does that?)

wdyt?

[sellout]

Yeah I forgot about these too, I don't use them currently.

Maybe they’re not worthwhile any more. I can see that with the pre-commit. After having it on for a while, I’m thinking it’s more annoyance than it’s worth. But I do have a similar¹ pre-push hook on all my repos.

I agree we shouldn't have to pass tests to commit; I'm not really sure what the right check is. ./scripts/check.sh is what I run when I'm trying to be careful; maybe that?

check.sh is a superset of stack build --fast --test. I could see using check.sh for pre-push, but it does take some time, since it runs all the transcripts, checks weeds, and checks formatting … but those are things that’d be nice to catch pre-publishing.

Another concern is that check.sh, test.sh, or any stack build command can change the build settings in a way that invalidates your build cache and thus slows down your next development build with your original settings.

Yeah, I find this very frustrating. I waffle between nix build and stack build, because Nix has to fully rebuild any package that’s changed while Stack can rebuild a subset … but Nix preserves past builds so if I pass through a state I previously built (like in an interactive rebase), I don’t have to rebuild anything.

The failure message should probably tell you how to bypass with --no-verify (unless git already does that?)

I don’t think Git tells you anything helpful when a hook fails, so yeah, that would be good to include.

Footnotes

1. But mine does nix flake check. ↩

[aryairani]

Just for keeping track, I'll mark this to hold for the hint about skipping checks; but I also don't mind switching it to Approve for now either.