Add a pre-commit hook to check all files

[hanzo]

#3521 added --new-from-rev HEAD to the pre-commit hook with the goal of making it easier to integrate golangci-lint into an existing code base without needing to address all existing lint errors. The problem with only checking changed files is that it makes it far too easy for new lint errors to be introduced: as soon as a developer runs git commit --no-verify they will never again be warned about the lint errors they just added. It's common to commit some incomplete code changes to save your progress before you're ready to polish the code and clean up every lint error, so you run git commit --no-verify and now the lint errors are permanently ignored.

The same is true for CI: a unit test that runs golangci-lint via pre-commit can now only warn about errors that were introduced in the most recent commit instead of the entire pull request. This also prevents linters from running against the entire codebase post-merge to catch lint errors that were caused by interactions between multiple PRs.

Instead of hardcoding --new-from-rev HEAD in the pre-commit hook, developers who desire this behavior can achieve this via configuration as described in this comment.

cc @davidjb @ldez - curious to hear your thoughts on this

[boring-cyborg]

Hey, thank you for opening your first Pull Request !

[CLAassistant]

All committers have signed the CLA.

[davidjb]

This PR shouldn't be merged.

@hanzo pre-commit is typically intended to run only on changed files (and the specific staged changes from those files) by default before a commit, so what you're describing as a concern is pre-commit's intended behaviour. If a developer is bypassing hooks in order to commit invalid code, that goes against the intent of pre-commit which is to catch issues ahead of commit (https://pre-commit.com/#introduction).

By forcing a check of all files in a repo during every pre-commit, you'll end up seeing unrealted linting errors which then prevent otherwise valid commits from being permitted. For example, if my commit changes only file A but unrelated file B has linting errors, checking all files means my commit will be blocked until I either fix file B, or use --no-verify -- both of which are non-solutions. If your intent is to avoid devs using --no-verify, then forcing a repo-wide check of unrelated files will see it used more (and/or your commits becoming less atomic).

pre-commit has an option for running hooks against all files, see https://pre-commit.com/#4-optional-run-against-all-the-files, but it sounds as though you just want to run golangci-lint over your whole code base. If so, then do this separately - just call golangci-lint directly, either locally or in CI; pre-commit is, as the name suggests, something that happens before a commit, rather than after.

If you really want your entire code base to be checked ahead of every commit, then I'd suggest creating a local hook with what's in your PR.

[hanzo]

Thanks for your reply, @davidjb. I'm aware of pre-commit's intended behavior.

To describe my organization's setup a little better: we have a microservice architecture with over 300 Go repos, and we want our linters to run the same way on every repo, whether you're running locally before committing or whether it's a CI check. It turns out that pre-commit works great for both, and we can easily manage the versions of golangci-lint and other linters in the .pre-commit-config.yaml file in each repo via automation. In every case we just use pre-commit run --all-files to run golangci-lint and other linters, and the same configuration is used both locally and in CI. We've already adopted a standard set of golangci-lint linters fleet-wide, and each of those 300 Go repos is fully conformant with the latest linter requirements. This setup has been working quite well for years. I recommend you try it!

I understand that this might not be the most common scenario for users of golangci-lint. The part that's not clear to me is why the behavior added by #3521 needs to be hardcoded in the pre-commit hook, when it could easily be achieved by configuration instead?

[aneeshusa]

When I originally added the .pre-commit-hooks.yaml in this repo (#311) I intentionally did not include --new-from-rev. pre-commit should catch the majority of issues locally, but it's explicitly designed to be used in CI - see their first-class https://pre-commit.ci/ service - and running it in CI lets you reuse the same linter definition instead of having a separate definition (e.g. running golangci-lint directly) which will get out of sync.

I'd also agree that enabling new-from-rev is an option downstream customers can enable easily - either in .golangci.yml, or by adding args: [--new-from-rev, HEAD] in their .pre-commit-config.yaml. Conversely, removing this flag is much harder (.golangci.yml config won't work as CLI args take precedence; can't use args in .pre-commit-config.yaml, must override the whole entry so blocked from getting future upstream fixes). I'd suggest we prefer correctness for varied workflows over optimizing for a specific workflow.

[ldez]

pre-commit is here to check the code before a commit, but this does not imply directly the scope of the check.
I can agree with @davidjb, a restricted scope to the commit can be expected.
But I can also understand that a restricted scope is not necessarily implied by the use of pre-commit.

Using new inside the configuration doesn't really seem a good approach because it will not be conditional.

The 2 points of view about pre-commit are understandable and don't allow a compromise.

I don't use pre-commit, maybe it's possible to define 2 hooks inside the pre-commit-hooks.yaml, one for each use case.

Note: using pre-commit inside a CI is, for me, a bad idea because this tool is a local tool to use before a commit.

[hanzo]

Thanks for your input, @ldez. I agree that it seems like the best path forward is to define two hooks.

Even ignoring the CI use case, --new-from-rev HEAD breaks linters like unused which need to run against the entire repo. I've updated my PR to create a new hook named golangci-lint-diff which uses --new-from-rev HEAD for those who desire this behavior. Any suggestions for a better name for the hook?

[ldez]

I would prefer the opposite:

• default: with new-from-rev same as today
• golangci-lint-full: without new-from-rev (not sure about the name)

[hanzo]

I've updated the PR as you suggested to add a hook named golangci-lint-full without new-from-rev.

I've also added descriptions for both hooks to make their behavior more obvious to future users. My team had to spend weeks undoing all of the lint errors that were introduced after #3521 silently broke our CI for months.

[hanzo]

@ldez thanks for updating the title. Anything else you'd like to see changed before this is ready to merge?

[ldez]

LGTM

[hanzo]

Hi @ldez, would it be possible to create a new release that includes this and the rest of the recent commits? I can't tell from the docs if anyone can create a release, or if this is only done by maintainers.

[ldez]

I will not create a release only for pre-commit.
I have some PRs to review, I will create a release after that.

---

Sponsoring is a good way to sustain open source maintainers: sponsor me