add support for nested gitignores - #2751
Conversation
mightyiam
left a comment
There was a problem hiding this comment.
These tests, did you see them fail before the implementation commit?
| pub fn walk_nix_files<P: AsRef<Path>>( | ||
| ignore: Gitignore, | ||
| target: P, | ||
| extra_ignores: &[String], |
There was a problem hiding this comment.
Looking at this... I'm not sure why there would be distinct ignore and extra_ignores as opposed to a single ignores. Thoughts?
There was a problem hiding this comment.
i guess we could probably refactor that another way to avoid having a "repeatition" but my idea was the following:
- we need a Gitignore object (
ignoreelement) at every step to know if the walker needs to traverse folders and files and everything - but we need to keep the full list of ignored stuff (
extra_ignoreselement) to rebuild a newignoreobject when encountering a new.gitignorefile in a nested subfolder
so i think we can't do without both? i may be wrong there but i don't see how to avoid this issue
There was a problem hiding this comment.
This discussion brings me back to thoughts I had before. Why do we have implementation of such common file traversal with gitignore logic in this project?
There was a problem hiding this comment.
what do you mean? surely we want to not check gitignored files, to avoid raising errors on files that are not relevant to the user
There was a problem hiding this comment.
What I mean is, isn't there a crate that "just does this" exact behavior?
There was a problem hiding this comment.
i didn't think about that earlier but i just saw that the regular ignore crate did..
I think we can just loop over their Walk iterator directly
https://docs.rs/ignore/latest/ignore/struct.Walk.html
This example shows the most basic usage of this crate. This code will recursively traverse the current directory while automatically filtering out files and directories according to ignore globs found in files like .ignore and .gitignore:
use ignore::Walk;
for result in Walk::new("./") {
// Each item yielded by the iterator is either a directory entry or an
// error, so either print the path or the error.
match result {
Ok(entry) => println!("{}", entry.path().display()),
Err(err) => println!("ERROR: {}", err),
}
}There was a problem hiding this comment.
Would you mind doing that, instead? Our test coverage should provide some confidence, right?
There was a problem hiding this comment.
sure, I'll try to look into it this weekend.
should I make a new PR?
There was a problem hiding this comment.
Great! Whatever you find most convenient.
|
i answered your points in the reply threads directly, feel free to ping me if needed
yes! nested gitignores were ignored (thus leading to statix fully analyzing every |
|
here's the new PR: #2756 with changes we discussed |
hello,
i tried to use statix in my repo and had issues with it checking ignored files / folders.
issue: statix only cares about the
.gitignoreat the root of the repo and not the nested ones (that I largely use)this PR support for nested .gitignore files when walking the repo, and adds a couple tests to make sure the patterns are applied relative to their directory.
the fix makes the walker pick up nested .gitignore files and apply their rules relative to the directory they’re in (if
unrestrictedis not true).disclaimer: this PR was largely made with support from gemma4-12b
all the tests are passing and i have confirmed expected behavior by adding 2 more tests and testing against my repo.
please tell me if anything needs fixing or if that's not an issue you're willing to deal with!
have a great day