-
Notifications
You must be signed in to change notification settings - Fork 23
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
Hide files excluded by .gitignore #32
base: main
Are you sure you want to change the base?
Conversation
This looks like a leftover from using another library for walking files. Signed-off-by: Timo Beckers <[email protected]>
Fixes hmarr#14 This commit adds support for hiding files from output that have been excluded by .gitignore and friends. I've implemented a few version of this and settled on just `git ls-files` since it's likely going to be the most correct and maintainable solution. I've tried github.com/boyter/gocodewalker but it's a complex piece of machinery and was much slower on a repo of 15k files (Cilium) than just git ls-files. I also tried out github.com/ianlewis/go-gitignore directly, but it doesn't pick up .gitignore files in subdirs automatically, nor the system-wide gitignore. I figured since the overwhelming majority of users will be running this in CI where Git will always be present, relying on the canonical .gitignore implementation (git itself) is the safest option. Signed-off-by: Timo Beckers <[email protected]>
Imho relying on the git cli command is not optimal. Refactoring |
Honestly, I wouldn't reach for something much more complex. It's pretty involved to implement correctly, although the codeowners lib already has fnmatch -> regex compilation, which is a large chunk of what's needed. Hard to tell if it works exactly like git's, though. If we use git as the source of truth for files to manage, there shouldn't be any surprises, it should just work. If someone wants to swap in a pure-Go implementation later, that's fine, but I personally don't think it's worth the engineering effort for a small tool like this. |
I am not the owner of this repo, but my 2 cents after reviewing the pr:
|
@rwese Can you provide an example of something that this change would break? |
@hmarr Ping in case you missed it, please let me know if you're interested in taking this patch. Will close in a week if no answer. |
Fixes #14