bazelbuild
feat: add support for .gitignore
What type of PR is this?
Feature
What package or component does this PR mostly affect?
all
What does this PR do? Why is it needed?
Allow users to opt-in to respecting the gitignore when collecting files for gazelle extensions to consume.
While bazel can access git-ignored files many repositories manually exclude git-ignored content by not including it in source file lists or globs in BUILDs or by duplicating the git-ignore entries as # gazelle:exclude.
Ideally users would put all bazel ignored content in .bazelignore but without glob support that is often not done.
This should remain opt-in for repositories wanting it but by default bazel can access git-ignored content so gazelle should as well.
Which issues(s) does this PR fix?
Fixes #1166
Other notes for review
@fmeum can you confirm if adding this to the gazelle core is something we'd like to do? If so are there preferences for which library to use vs manual?
Functionally gitignore also works a bit different with things like ! operators etc. That is one reason to use a library such as go-git instead of doing it ourselves but we could also potentially do it ourselves but don't support !...
I kind of like this as it reduces the amount of Gazelle-specific configuration.
I don't know which library is best, but we should definitely use one rather than roll our own.
I don't know which library is best, but we should definitely use one rather than roll our own.
I agree we should use a real library and not try to re-implement gitignore logic, otherwise we'll just end up with a never-ending stream "it doesn't work like git" bugs.
The go-git library is what I've used in all the aspect gazelle extensions and I've never had any issues so I think that's a good place to start. If we add appropriate tests then we should be able to change the implementation later.
FWIW I've also tried other libraries which all had issues in the aspect gazelle extensions:
- https://github.com/sabhiram/go-gitignore - too slow, don't recall if there were other bugs
- https://github.com/denormal/go-gitignore - had multiple bugs (https://github.com/aspect-build/aspect-cli-legacy/issues/88)
Those both also seem to be unmaintained.
Others I have not tried seem to also be unmaintained and many look too simple or slow:
- https://github.com/zabawaba99/go-gitignore
- https://github.com/src-d/go-git
- https://github.com/zabawaba99/go-gitignore
Based on https://github.com/aspect-build/aspect-cli-legacy/issues/52, what do you expect to be the use case for this if we can't enable it by default due to performance issues? It's surprising to me that .gitignore processing should be costly - do you know what the hotspots are in the current implementation?
I was suggesting it should be disabled by default because that is the ideal use case for both "correctness" and performance (although performance is only really a concern in very large projects globbing on hundreds of thousands of files).
However enabling it by default is probably a better initial experience for most projects so I'm fine with that as well.
Longer explanation, imo...
The ideal use case is when bazelignore specifies everything bazel should ignore, and gitignore specifies everything git should ignore. Those are not always 1-1... sometimes bazel might need to process things that are git ignored, very often things such as IDE config are in git but should be ignored by bazel etc.
In reality the more common use case is projects use gitignore and then (partially) duplicate that in glob([exclude]) + # gazelle:exclude, and bazelignore is almost unused. This is done either due to lack of awareness of bazelignore or the fact that bazelignore does not support globs so it is too much of a hassle or sometimes not even possible.
This feature would remove the need for many of those glob([exclude]) + # gazelle:exclude which I think makes this feature worth it for most projects. I essentially agree with everything in this comment: https://github.com/bazel-contrib/bazel-gazelle/issues/1166#issuecomment-1024595950
@fmeum has your opinion changed now that bazel8 has ignore_directories? See https://github.com/bazel-contrib/bazel-gazelle/issues/1994
We could drop this PR and tell people to use ignore_directories instead which really is the "correct" solution - gazelle should ignore the same things bazel ignores (anything extra should be a gazelle api such as # gazelle:exclude).
The ignore_directories is just a bazel glob() like in BUILD files so it is much simpler then gitignore, we can just use doublestar like we already do for # gazelle:excludes instead of this go-git library in this PR etc.
I appreciate the effort you put into this, but yes, I think that we should support ignore_directories instead. As you remarked, it's simpler but still expressive enough to be generally useful. We could also enable it by default without any concerns about that excluding too much or too little.
Here's a quick ignore_directories implementation: https://github.com/bazel-contrib/bazel-gazelle/pull/2039