Fix install-hooks.bash script #5910
Conversation
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`). 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.
- Bash “strict mode” - address ShellCheck complaints - replace backticks with `$()` - don’t compare against `$?`
|
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. Another concern is that The failure message should probably tell you how to bypass with wdyt? |
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 similar1 pre-push hook on all my repos.
Yeah, I find this very frustrating. I waffle between
I don’t think Git tells you anything helpful when a hook fails, so yeah, that would be good to include. Footnotes
|
aryairani
left a comment
There was a problem hiding this comment.
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.
This also sends the error output to stderr.
|
Merged trunk to get updated GitHub runners. |
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
core.hooksPathis set (i.e.,$GIT_DIR/hooksisn’t the right place),-f).Implementation notes
This also replaces
ln -swithcpso 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-verifyto 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
pushmeans you’d want to be more careful about it.