4 months ago
I'm of the opinion that Git hooks are personal. That's why they're not part of the source code itself. I make extensive use of hooks, but they're tailored to my needs.
Note that you can skip hooks by passing the --no-verify flag to subcommands. Comes in handy when they're slow and you know that you've just fixed the wrong formatting that the previous invocation of your pre-commit hook complained about.
4 months ago
pre-commit hooks are awful, they get in the way of interactive rebasing. pre-push is where they belong; save a round-trip through CI.
4 months ago
You can check inside the hook if you're in the middle of the rebase, and exit the hook early.
This is what we have in our hooks:
if [ -d "$(git rev-parse --git-path rebase-merge)" ] || \
[ -d "$(git rev-parse --git-path rebase-apply)" ] || \
[ -f "$(git rev-parse --git-path MERGE_HEAD)" ]; then
exit 0
fi3 months ago
Thanks for this, I can’t believe this never occurred to me to try to do.
4 months ago
I would argue that if the pre-commit hooks come in the way of rebasing, either the commit hooks are doing way too much (which is one of the points of the article) or you are creating broken commits during rebasing. If any of the commits you are rebasing is e.g. breaking formatting rules, they shouldn't have been committed that way in the first place.
4 months ago
I personally avoid pre-commit hooks and instead use git rebase -x "hook" to ensure my commits are not broken
4 months ago
> I would argue that if the pre-commit hooks come in the way of rebasing, either the commit hooks are doing way too much (which is one of the points of the article) or you are creating broken commits during rebasing.
I don't think your argument is grounded on reality. Applying whitespace changes does create merge conflicts, and if you have a hook that is designed to introduce said white changes at each commit of a rebase them you are going to have frequent merge conflicts.
Keep also in mind that minor changes such as renaming a variable can and will introduce line breaks. Thus even with a pristine codebase that was formatted to perfection you will get merge conflicts.
> If any of the commits you are rebasing is e.g. breaking formatting rules, they shouldn't have been committed that way in the first place.
You're letting the world know you have little to no programming experience.
4 months ago
I love hooks in general but I definitely have a thing against hooks that aren't read-only (except maybe some autogenerated stuff that "can't go wrong").
The hooks can fail my commit all they want, but I don't want them actually changing anything I've done, which definitely implies no reformatting in hooks.
4 months ago
maybe, because i really don't have the problem you're describing.
yes, formatters run on every commit. not only during rebase, but also every commit beforehand. if that is done consistently, the formatter does not cause merge conflicts.
merge conflicts during rebases due to variable name changes occur without commit hooks, too.
4 months ago
> if you have a hook that is designed to introduce said white changes at each commit of a rebase them you are going to have frequent merge conflicts.
Only if you rebase commits prior to the introduction of the hook. Otherwise that whitespace change should be already there in the old commits.
4 months ago
Yeah there's really no trouble with a pre-push hook that runs common/fast code checks. For TS projects I just run formatting, type, and lint checks. Faster feedback than spinning up a runner in CI and if I don't need it I just tack on --no-verify.
4 months ago
> For TS projects I just run formatting, type, and lint checks.
For formatting I find that it's clearly preferable to lean on the IDE and apply the source code formatter at each file save, and apply it only to the file you are touching. Type checks should be performed right before running unit tests, for the same reason unit tests are executed.
4 months ago
Yeah of course, same here for formatting. But in the world of LLM agents it's pretty easy to have formatting issues sneak by without the IDE being involved. It's a very quick and easy check.
For type checking, I guess that makes sense if your unit tests are small and quick. At work type checking our entire codebase can take like 10-15 seconds with a cold cache, but running all unit tests takes 20 minutes (and multiple shards in CI). Seems like a no brainer to just run the cheap one more often.
4 months ago
Database integrity constraints fall into the same category.
This entire class of automation is awful and defeats the robustness of the tool itself.
All of these things have terribly unpredictable consequences and tend to fail at the worst moments, such as during a SEV.
You can encode the same rules and discipline in other ways that do not impact the health of the system, the quality of the data, or the ability of engineers to do work.
4 months ago
4 months ago
agents trip up over them too.
4 months ago
Can't stand pre-commit hooks. I want zero delay on commits. Checks can be run against pull requests in a GitHub action runner; no reason to force me to run them on my machine.
4 months ago
zero delay on commit, but then an entire CI run and feedback loop just to fix a linter or formatting issue
4 months ago
The solution could be a pre-push hook. I am also not a fan of pre-commit hooks because I just want to commit my wip changes. Not stash. Commit.
It's fine if the auto formatting tool hasn't been run. If the pre-commit hook changes my files silently, that is a big no-no for me.
I have had tools break things before and it makes it very hard to work out what happened.
Having it fail to push means I get to choose how to fix up my commits - whether I want a new one for formatting changes etc or to go back and edit a commit for a cleaner public history.
4 months ago
I used to think pre-push was better than pre-commit but at some point I realized I was actually just kicking the can down the road and leaving bigger problems for myself. It's not downsides-free, but it's the better compromise for me.
100% agree on hooks being readonly.
Username oddly relevant to context btw.
4 months ago
Couldn't disagree more.
Waiting for a CI step to tell me something's wrong when I could've found out locally is a waste of time.
Sure, I can hand-run checks locally, but having a way of doing it "automatically" pre-push gives me consistency and saves time.
4 months ago
That's fine, but it shouldn't be enforced on all contributors. What matters is that failures don't get merged, not that they don't get committed.
4 months ago
Yeah.
As long as "don't get merged" includes squashing so that whatever your (non)hooks didn't catch locally don't end up causing failures for rebases/merge conflict resolutions for others (assuming there are repo-level hooks people are expected to be using).
4 months ago
Pre commit hooks shine with fast formatters. Keep the hook under half a second or so and it's great.
4 months ago
it's the tools. if they're slow people disable them or shift them right. you want your defect-detection and fixing to shift left
if you're not running auto-format on file-save, your auto-formatter is slow
if you're not running a code checker with auto-fix on pre-commit, your code checker is slow
if you're not running the test-suite on pre-push your tests are slow
if your tooling is slow you need to pick better tooling or make them fast
you want to keep that loop tight and active
4 months ago
> if you're not running the test-suite on pre-push your tests are slow
Of course tests are slow. If they're fast they're skipping something. You probably want to be skipping something most of the time, because most of the time your change shouldn't cause any side-effects in the system, but you definitely want a thorough test suite to run occasionally. Maybe before merge, maybe before release, maybe on some other schedule.
Each individual change might be fine in isolation but cause excessive memory pressure when accumulated with other changes, for example. Unit tests won't catch everything, integration & functional (hardware in-the-loop) tests are needed. I even sometimes have to run tests in a thermal chamber repeatedly to cover the whole -40-105°C temperature range, since the firmware I work on runs on hardware that allows that range & performance varies with temperature.
4 months ago
> but you definitely want a thorough test suite to run occasionally.
that's what ci is for
anyway, if you're doing embedded development and can distinguish between different kinds of testing then my previous post is not meant for you
4 months ago
What is a good time to run slower tests? My full test suite takes around 4 minutes to run, and it’s trending upwards. I am a solo developer, so I run it pre-push.
And what about tests that only need to run intermittently, such as broken link checkers?
4 months ago
your ci setup should take care of the slow tests — for most folks this is github actions
pre-push can run the faster unittest set, ci will run the integration set
aim to keep pre-push duration under 10s
4 months ago
Is there a reason for that? I figure that catching bugs before pushing is faster than getting an email from GitHub, and my hardware already has all the containers running. Spinning up machines in the cloud for this feels wasteful.
4 months ago
if you want to reuse the local setup for ci or pushing broken code is somehow undesirable, then run all the tests pre-push
i tend to run only the fast set of tests, or a subset of the environments matrix, on pre-push — CI can take care of the rest while, i move onto do something else
when working with other the CI status is what matters
4 months ago
> if you're not running auto-format on file-save, your auto-formatter is slow
Or you care about your formatting and don't want to throw them away.
4 months ago
i don't know what you think i care about, when i run the auto-formatter on file-save, if not for the formatting of the code
4 months ago
Certainly not the formatting you have done manually, because the auto-formatter will likely throw it away. You likely do that, because you care about the consistency of the formatting, not for the formatting already present in the file.
Personally, I don't like auto-formatters. It's like having someone else cleaning up your room. I have put every byte of whitespace where it belongs, I certainly don't want someone else to mess that up.
4 months ago
I use git hooks in professional setting to prefix every commit message with the ticket the commit belongs to (this is done by extrapolating the ticket id from the feature branch name). It works pretty well and has saved my bacon a couple of times when I needed to figure out the provenance of a particular commit. Especially useful when I had to ask why that particular change was necessary.
4 months ago
The main thing I've used them for is to prevent me committing to 'main', as we do everything via Merge Requests.
But it requires me to remember to set that up for each repo, and this is a massive pain. I'd like to be able to have a "clone with hooks" option, but I don't think anyone has found a way of making that work well without leaving people in danger when they clone a random repo.
4 months ago
Isn't that what branch policies are for? It can get annoying when making a release (from a local machine as opposed to automatically in CI/CD pipelines), but in other circumstances it serves the purpose very well in my experience
4 months ago
Ooh, how do those work locally?
I've only encountered those on the server side.
4 months ago
git itself has no concept of branch policies, it is purely a server side thing. But surely it doesn't really matter what branch you commit to locally, if you can't push it, you haven't done any damage and can just create a new branch and push that instead?
4 months ago
Yes, but I'd like to avoid the "create a new branch, switch back to main, reset main back to origin, come back to the new branch" dance. And a git hook does that, but it's not trivial to set up (particularly when there are lots of repos).
4 months ago
Maybe create a shell alias which would act as a wrapper around git to do just that, when you try to commit on the wrong branch
4 months ago
read the article, it recommends devenv or pre-commit as tools that do thr setup for you
4 months ago
Alas, neither works for me. I've not managed to find a Windows equivalent yet.
Edit: `git config --global core.hooksPath D:\GitHooks\` is what I needed!