-
Notifications
You must be signed in to change notification settings - Fork 102
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
gitlint is showing all files as changed in github CI #338
Comments
Found the issue. By default the checkout action only checks out the commit being modified. In gitlint, we basically invoke gitlint/gitlint-core/gitlint/git.py Line 369 in 8276aec
It's the
We added the While this is easily fixed in CI by setting Need to brainstorm and research a bit whether that means we shouldn't show any changed files when linting the initial commit (i.e. remove |
So when a single commit is checked out from git (a shallow clone), its parent information is lost. Typically, the way to determine whether a commit is one of the initial commits on a repo, is by checking whether the commit has parents using a variation of: > git rev-list --max-parents=0 HEAD
765a4427f80ccdda61bc78e2680bda14787b2a4e # this is the sha of the initial commit We could have used the output of this to figure out when to include the I did find out that such shallow commits that are not initial commits have a so-call While technically it might be possible to use this grafted annotation to figure out whether any given commit without parents is an initial commit or not, and depending on that use the In addition, it’s worth calling out that gitlint actually doesn’t adhere to the same semantics as git does by default, i.e. git by default won’t show you files that were part of the initial commit as being changed files. As such, I’m inclined to revert gitlint behavior on this to align with default |
The same behaviour applies to Gitlab CI. |
As a side note, I am stuck on 0.15 now because of that issue and this means I cannot run a recent version of black which depends on click>=8 while gitlint 0.15 pins click to version 7. |
@jorisroovers FYI here's an example of using GitHub API to figure out how many commits a PR has (up to a certain limit, w/o pagination) cherrypy/cheroot#579. That PR is only implemented for |
I would very much want this to work agnostic of the Git provider as I am using GitLab in a corporate environment. |
@chbndrhnns this won't be possible for as long as you want to only lint commits from PRs/MRs. Each of the providers needs to be handled separately. Figuring out what to lint may be embedded into gitlint, technically, but this would only work with envs for which it'll be implemented, any new/exotic cases would need to be handled in the job definitions outside the call to gitlint. |
FWIW, I don't use GitLab actively these days, but I'm happy to consult on/review the GHA-related things @jorisroovers. |
I understand it used to work up until gitlint 0.15 so "won't be possible" seems a bit too strong. Also, one comment above outlines a solution that sounds compatible to me:
|
I guess, what works and what doesn't, depends on the PoV. Of course, you could track down the common ancestor for two branches, but usually CIs don't check out the whole Git tree. They get the history up to certain depths. For GitHub this defaults to 1, for Travis CI it used to be like 200 commits back IIRC. So there are some bits that won't work w/o certain pre-setup. I suppose that gitlint won't want to be intrusive, so automatically fetching more history is not an option in this case and would require some pre-setup from the users. |
I get that now thanks to your explanation, so thanks! |
I realize I didn't mention earlier that there’s really 2 parts to this. Initial Commits and the git diff-tree --root flagAn important detail worth repeating is that git itself doesn’t consider files of the initial commit as “changed files” when you do I had added the Since I’ve now discovered that it’s not possible for gitlint to (easily + accurately) determine whether a commit is root commit, I believe gitlint should align with git’s behavior and not show files for initial/root commits unless you explicitly tell it to, with a config option like: [general]
# consider files of root/initial commits as changed files
# (option name subject to change)
diff-tree-root=False # disabled by default You don’t actually have to use a By using this construct, you can have an if/else statement in CI that deals with the initial commit separately if you really want it to. I think most users won’t actually care for the initial commit. We could documenting and/or provide templates on how to do this in various CI systems. Fetch depthHowever, this The only way to resolve this AFAICT, is to increase the fetch-depth, which is CI specific. I don’t want to include CI specific customizations in gitlint itself, but perhaps we can also make this better using documentation and/or templates. To reproduce this locally: # Fetch depth of 1
$ git clone --depth 1 https://github.com/jorisroovers/gitlint.git /tmp/gitlint
$ cd /tmp/gitlint
$ git diff-tree --no-commit-id --numstat -r 01f2fa0
# No output :(
$ git diff-tree --no-commit-id --numstat -r --root 01f2fa0
# All files are listed :(
$ rm -rf /tmp/gitlint
# Fetch depth to 2(or more): fixes this issue
# Whether you use --root or not, only matters for the initial commit (not shown here)
$ git clone --depth 2 https://github.com/jorisroovers/gitlint.git /tmp/gitlint
$ cd /tmp/gitlint
$ git diff-tree --no-commit-id --numstat -r 01f2fa0
1 1 gitlint-core/setup.py
$ git diff-tree --no-commit-id --numstat -r --root 01f2fa0
1 1 gitlint-core/setup.py
$ rm -rf /tmp/gitlint |
Gitlint is incorrectly listing all files as changed in CI, not only those that have changed:
https://github.com/jorisroovers/gitlint/actions/runs/3014300199/jobs/4935936697#step:14:104
Is this perhaps due to a misuse of refspec? Should we be passing a
-1
argument like we do withlog
?The text was updated successfully, but these errors were encountered: