Comment by ufo
4 months ago
I have always had this problem with hooks and new contributors: since hooks don't run by default if you just clone the repository, my open source projects get many PRs from new contributors that did not run the linting and commit hooks. I understand there's a security reason for this but what workflows have worked best for you to get everyone to run the hooks? And do you think the new config-based hooks can help new contributors?
> what workflows have worked best for you to get everyone to run the hooks
By running the linters and any other checks on CI instead.
Why waste a round trip, build time, loss of flow and CI machine queue wait time when you can catch things early?
CI should also run all the checks but CI checks are not a replacement for local hooks. LFS and things like it can't be implemented as remote CI checks.
Why are we acting like a James Bond villain, slowly lowering the changes into the vat of sharks after we've left the room? I want the hooks. Can we talk about making that easy, assuming some people want them?
> Why waste a round trip, build time, loss of flow and CI machine queue wait time when you can catch things early?
Because we want to be sure that the checks have passed, and that they have passed in a clean environment.
Contributors can, in addition, use git hooks, or run tests in watch mode, or use their IDE.
Also it's annoying to have slow git hooks if you commit often.
You're looking for a technological solution for a human problem.
Automatically running arbitrary code from random repositories is a Really Bad Idea, so Git will almost certainly never auto-install pre-commit hooks. Just mention it in the README and run a checker in CI to confirm they are using it, it really isn't that difficult.
People wasting 2 minutes of their own time once during their first contribution because they didn't read the README is not that big of a deal. What's next, you want a script to automatically sign a project's legally-binding CLA on checkout?
2 replies →
Local hooks are just a convenience. CI checks are assurances, you have to have them.
If one hates the round-trip he/she will adopt hooks quickly.
3 replies →
autoformatter and autofix linter results can be committed and pushed by CI into the PR branch itself. this is a pain sometimes, but as a repo owner it should protect your sanity.
Yep. Nothing I hate more than some trivial formatting error that could easily fix itself halting CI. I am all for consistent formatting and linting, I just think it should be silently handled without fuss.
I just add a check workflow that test that the files are well formatted and linted. If it passes, one of the key things I check are changes to the configuration. Some tools allows for bypass comments, so I keep an eye out for those too.
We do run the linter on CI as well, but I think our comitters would get faster feedback if they ran those checks locally.
Well you can tell them to please enable hooks in the PR guidelines, but you cannot really police what they do or don't run on their own machines.
3 replies →
You can issue installation instructions on linter failure in CI.
As well, not instead. Just add `pre-commit run -a` to your CI. Job done.
It's still annoying for new contributors though because they might not know how to set up pre-commit (which was quite a pain until recently because it's written in Python).
To clear up any confusion, Git runs pre-commit hooks, and they can be written in any programming language. There's a completely separate and independent project that gave itself the confusing "pre-commit" name, and it is written in Python. This project aims to make it easier to configure pre-commit hooks. An alternative to it is "prek", written in Rust.
3 replies →
> By running the linters and any other checks on CI instead.
Running linters on CI is an antipattern if there was ever one. That and configuring pipeline runs to fail for linting issues.
Sometimes some people just want to create their own problems. Configuring the editor solves most of the problems, and hooks add a failsafe. Once the code is committed, it should be immutable.
Many projects have used "hook managers" like Husky for this to install hooks to run based on repository-stored metadata.
These new config-based hooks are definitely a step down the road towards obsoleting the third-party hook managers. One of the things they are for is for managing scripts to support multiple hooks for the same event. The new config format supports multiple hooks for the same event natively. (Which also helps in stacking personal ones versus repository ones.)
The only thing missing is that the repository's .git/config isn't itself source controlled in that repository, so for now there would still be an "install step", but it's now a lot simpler of an "install step" with the install being "append .example-gitconfig into your .git/config" rather than "set X files to X different contents in .git/hooks/*" where X is the number of event hooks to be concerned about.
It does open the door further to if there should be a ".gitconfig" in the Repository working tree that can also contribute repository-wide shared config, what config it can or cannot contribute, and how you secure that as a reviewable opt-in (especially change notifications). But a smartly built secure UX there would be a massive improvement over, say, shell scripts in npm postinstall operations touching .git/hooks "for you" (which is how many of the current hook managers auto-install, as side effects in dependency installs).
(ETA: Though most "install scripts" now just use the very scriptable `git config` command and so just be `git config set --local hook.$name.$field $value` sequences, which is also a simpler improvement over previous ways to install and/or merge hooks files by hooks managers.)
I wish that git would auto check for a `.githooks` directory in the repo root and prompt on first clone if the `core.hooksPath` should be changed for this repositry and when pulling any tracked file in hooksPath causes a warning (though this still leaves out the case that some hook just invokes a script in the repo outside the dir).
I don't want you to run arbitrary hooks on my machine. As with CI/CD... your hooks should simply point to a script instead
Ok well what about when I pay you and give you a local machine to work on?
Can I pay you to run hooks on the work machine I own because it saves a lot of work on the share build machines? Can we talk about making that situation less error prone?
Tools growing unexpected code execution is how we keep having problems with secrets and other important things being stolen. If you add this feature to git, generally, then anybody cloning a git repo is going to have to deal with the fact that `git clone` might run arbitrary code. `git clone` is like `cp`. Do you want `cp` to unexpectedly run code? It should never do that.
Why force git to be a build tool?
Just document how to execute the scripts/checks that will be used by ci. Provide a simple script in the repo that folks can intentionally execute.
1 reply →
Yes that’s perfectly fine of course. But these days that’s not so common
I add an autogen.sh script to all my repositories that does things like this as it's first action.
You can also set up a central git template repository, so hooks get automatically added into every repository you clone
I always considered hooks a nice to have feature for devs to already validate that their PRs will probably satisfy certain CI checks. If they don't install or run them for whatever reason, it's on them to do another iteration and update the code to make it mergeable if CI complains. So I usually considered it fine that they are only opt-in, since the merge will be gated by a CI outside of the dev's control anyway.
In PHP, an established tool is adding GrumPHP [0] to your dependencies.
It will then handle git hooks on each commit via composer script by default (but can be omitted per commit).
[0] https://github.com/phpro/grumphp
Adding configuration to the config makes things feel far less exotic. I think these changes certainly improve things, but there's still plenty of room to go further.
I think there should probably be a way to specify canonical git configuration for things like hooks and LFS and all of that. It would be nice if when you clone, git prompts you to trust the remote config or to ask you to accept each new change as they come or fully reject them.
Having to scrape through the readme of every repo and then run arbitrary scripts doesn't seem like the most secure solution. When there's a canonical flow gitlab GitHub and all the tooling can support it and have proper permissions around it.
It's really disappointing how much lack of empathy there is when talking about new git features. Forget empathy. There's outright disdain for discussing alternative workflows.
The approach some JS projects have taken is to use Husky, which automatically sets up the git hooks when you install the project's dependencies during development.
> I understand there's a security reason for this
While I understand the security concerns, too, wouldn't this be solved by including a `trust` command like in direnv and mise? (I.e. have the user review the hooks before executing them for the first time.)
My project needs other things on setup as well, so I just have a setup script in my repo. `mv hooks/foo .git/hooks` is then just yet another step.
I agree with the other replies saying to just run the checks in CI and have the CI error message mention how to install the pre-commit hook.
I'm glad cloning a repo doesn't automatically install hooks since I strongly dislike them: I often use Git commands in the terminal but sometimes I use the VS Code UI to commit, and it's extremely frustrating when simply creating a commit runs for several seconds because of some pre-commit hook.
There’s almost certainly a way to make VS Code use --no-verify.
Use a dev container