Skip to content
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

Directories with trailing slash in .gitignore will not be exluded in version 6.17 #3524

Closed
hille721 opened this issue Jun 1, 2023 · 5 comments · Fixed by #3527
Closed

Directories with trailing slash in .gitignore will not be exluded in version 6.17 #3524

hille721 opened this issue Jun 1, 2023 · 5 comments · Fixed by #3527
Labels
Milestone

Comments

@hille721
Copy link
Contributor

hille721 commented Jun 1, 2023

Summary

directories with trailing slashes in .gitignore won't be exluded in ansible-lint 6.17.0.
I guess this is related to the reimplementation of the file exclusion logic (#3507).

This statement

return any(spec.match_file(str(path_to_check)) for spec in pathspecs)
will return false for a comparison of Path(foo) with foo/ in the pathspec object. It will even return false for Path('foo/') as pathlib will strip trailing slashes (see python/cpython#65238)

Issue Type
  • Bug Report
OS / ENVIRONMENT
ansible-lint --version
ansible-lint 6.17.0 using ansible-core:2.14.6 ruamel-yaml:None ruamel-yaml-clib:None
  • ansible installation method: pip
  • ansible-lint installation method: pip
STEPS TO REPRODUCE
echo "foo/" > .gitignore
mkdir foo
echo 'foobar' > foo/bar.yml
ansible-lint -v

This will result in following error:

INFO     Loading ignores from .gitignore
WARNING  Listing 1 violation(s) that are fatal
load-failure: Failed to load or parse file.
foo/bar.yml:1


             Rule Violation Summary              
 count tag          profile rule associated tags 
     1 load-failure min     core, unskippable    

Failed after : 1 failure(s), 0 warning(s) on 2 files.

As you can see, it won't be excluded.

Desired Behavior

It should work the same as if the directory is specified without trailing slash:

echo "foo" > .gitignore
mkdir foo
echo 'foobar' > foo/bar.yml
ansible-lint -v
INFO     Loading ignores from .gitignore
INFO     Excluded: foo

Passed with production profile: 0 failure(s), 0 warning(s) on 1 files.
@hille721 hille721 added bug new Triage required labels Jun 1, 2023
@hille721
Copy link
Contributor Author

hille721 commented Jun 1, 2023

easy fix would be for example to change the lines

pathspecs.append(
pathspec.GitIgnoreSpec.from_lines(f.read().splitlines()),
)

to

pathspecs.append(
    pathspec.GitIgnoreSpec.from_lines(line.strip("/") for line in f.read().splitlines()),
)

@ssbarnea
Copy link
Member

ssbarnea commented Jun 1, 2023

@hille721 Thanks for the very well written report! Would you mind making PR to address it?

@ssbarnea ssbarnea added this to the 6.17.x milestone Jun 1, 2023
@hille721
Copy link
Contributor Author

hille721 commented Jun 1, 2023

--exclude foo and --exclude foo/ also have a different behavior. Both are working but with --exclude foo/ ansible-lint will look into foo directory first and then exclude all file individually. This could lead to a light performance impact on large directories.

$ ansible-lint --exclude foo/ -v
INFO     Excluded: foo/bar.yml
INFO     Excluded: foo/bar2.yml

Passed with production profile: 0 failure(s), 0 warning(s) on 0 files.


$ ansible-lint --exclude foo -v
INFO     Excluded: foo

Passed with production profile: 0 failure(s), 0 warning(s) on 0 files.

@hille721
Copy link
Contributor Author

hille721 commented Jun 1, 2023

ok found a similar issue, which the guys from black project had: cpburnz/python-pathspec#65
pathspec version 0.10.3 introduces a new method pathspec.util.append_dir_sep() which we can use in the is_excluded function to fix this issue.
Will do some more tests and provide a PR.

hille721 added a commit to hille721/ansible-lint that referenced this issue Jun 1, 2023
hille721 added a commit to hille721/ansible-lint that referenced this issue Jun 1, 2023
@ssbarnea
Copy link
Member

ssbarnea commented Jun 1, 2023

That is brilliant, I wanted to suggest you to look at black as they are heavy users of pathspec.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants