jorisroovers / gitlint

Linting for your git commit messages
http://jorisroovers.github.io/gitlint
MIT License
813 stars 99 forks source link

gitlint cannot be run easily via pre-commit inside a CI without pre-commit configuration for the commit stage #191

Open guillaumelambert opened 3 years ago

guillaumelambert commented 3 years ago

Some projects maintainers require that all linters must be installed via pre-commit ant its YAML configuration, even when they are called from a CI. Typically they want to use a tox profile simply calling " pre-commit run --all-files --show-diff-on-failure".

This is not possible with gitlint since it only proposes a pre-commit configuration for the commit-msg stage with different defaults than the one recommended in a CI, mainly because a .git/COMMIT_EDITMSG file is needed.

When running pre-commit in a CI system the COMMIT_EDITMSG does not (normally) get created as that is an artifact of editing the commit message. If the file does not exist then gitlint will skip which makes it possible for pre-commit checks that should fail, to pass.

A workaround is proposed by the global-jjb project at https://gerrit.linuxfoundation.org/infra/c/releng/global-jjb/+/67641 to generate such a file but it had several drawbacks such as:

Indeed, the issue should better be fixed in gitlint uptream repository itself so that gitlint can be run at the commit stage from pre-commit. Since gitlint embeds its own hook solution for this stage, this configuration is not (yet?) proposed in gitlint official repository. https://jorisroovers.com/gitlint/#using-gitlint-through-pre-commit

Also the pre-commit hook provided up to now has a different behavior than standalone gitlint defauts settings that are advised in a CI context (i.e. "gitlint lint" or simply "gitlint"). https://jorisroovers.com/gitlint/#using-gitlint-in-a-ci-environment

The parameters --stage and --msg-filename options are passed and they appear to be the root cause of the problems in the CI since they add new requirements i.e. a local git user configuration and the presence of an extra file argument. https://github.com/guillaumelambert/gitlint/blob/main/.pre-commit-hooks.yaml introduced in commit 9de1f89b8697809713bce0eadf25fdd578b1d844

pre-commit hook YAML upstream configuration does not allow to use different options depending on the stage inside a same id block nor with two blocks using the same id. Only options from the last block appears to be used in that case. As a consequence, a new id "gitlint0" must be declared but it still can use the same name "gilint" displayed at runtime.

dupuy commented 8 months ago

I'm using the https://pre-commit.ci CI service to run gitlint as a pre-commit check, but had to explicitly add stages: [manual, pre-commit] to get it to run there.

The .pre-commit-hooks.yaml added in commit 973db5d has stages: [manual] which doesn't add the manual stage, but rather replaces the default pre-commit stage with manual.

Since the gitlint hook only runs in the commit-msg stage, and gitlint-ci only runs in the manual stage, adding the gitlint-ci hook has no effect for pre-commit.ci without making other changes – the minimal change is to add stages: [manual, pre-commit] to the hook configuration in my repo's .pre-commit-config.yaml.

guillaumelambert commented 8 months ago

Hello You can find the debate about manual vs pre-commit stages in the related PR comments https://github.com/jorisroovers/gitlint/pull/192 Hope this helps Guillaume

I'm using the https://pre-commit.ci CI service to run gitlint as a pre-commit check, but had to explicitly add stages: [manual, pre-commit] to get it to run there.

The .pre-commit-hooks.yaml added in commit 973db5d has stages: [manual] which doesn't add the manual stage, but rather replaces the default pre-commit stage with manual.

Since the gitlint hook only runs in the commit-msg stage, and gitlint-ci only runs in the manual stage, adding the gitlint-ci hook has no effect for pre-commit.ci without making other changes (the minimal change is to add stages: [manual, pre-commit] to the hook configuration in my repo's .pre-commit-config.yaml.

dupuy commented 8 months ago

Adding the pre-commit stage for the gitlint-ci hook to my .pre-commit-config.yaml was a pretty easy fix to get the hook to run on @asottile's excellent pre-commit.ci service. My PR was mostly to ease the way for others who might want to run this check in that environment.

But it turns out that for the pre-commit.ci service, there is a larger problem I couldn't resolve. As a low latency, low overhead CI service, pre-commit.ci avoids extra overhead and cost by checking out the Git repo for a PR as a shallow (depth=1) copy, and I could not find any way to deepen the repo, despite several different attempts. Even if the full branch commit history wasn't present, I thought there might be some benefit in checking the final (and only) commit message. However, even that minimal check was stymied, since the final commit in the shallow copy at pre-commit.ci is a merge commit between the branch and trunk, with a standard machine generated commit message that isn't worth checking. At that point I decided to abandon the effort, and added the gitlint-ci hook to the set of pre-commit hooks skipped by pre-commit.ci.

guillaumelambert commented 8 months ago

Adding the pre-commit stage for the gitlint-ci hook to my .pre-commit-config.yaml was a pretty easy fix to get the hook to run on @asottile's excellent pre-commit.ci service. My PR was mostly to ease the way for others who might want to run this check in that environment.

But it turns out that for the pre-commit.ci service, there is a larger problem I couldn't resolve. As a low latency, low overhead CI service, pre-commit.ci avoids extra overhead and cost by checking out the Git repo for a PR as a shallow (depth=1) copy, and I could not find any way to deepen the repo, despite several different attempts. Even if the full branch commit history wasn't present, I thought there might be some benefit in checking the final (and only) commit message. However, even that minimal check was stymied, since the final commit in the shallow copy at pre-commit.ci is a merge commit between the branch and trunk, with a standard machine generated commit message that isn't worth checking. At that point I decided to abandon the effort, and added the gitlint-ci hook to the set of pre-commit hooks skipped by pre-commit.ci.

AFAIK, shalow (depth=1) is gitlint default config and fits well in Gerrit because each commit is reviewed separately. With github and gitlab, the PR can contains several commits, some projects CI use git shallow option to purge the git history but this is normally configurable. If this is not the case, did you try anything like " pre-commit run --all-files --show-diff-on-failure --from-ref deadbeef123 --to-ref HEAD~" ?