feat: add support for .gitignore #1908
Conversation
|
@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? |
|
How is this different from using 'gazelle:exclude' directives? |
|
People won't want to copy+paste gitignore files into BUILD files...? |
|
Functionally gitignore also works a bit different with things like |
|
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 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 |
|
FWIW I've also tried other libraries which all had issues in the aspect gazelle extensions:
Those both also seem to be unmaintained. Others I have not tried seem to also be unmaintained and many look too simple or slow: |
jbedard
force-pushed
the
gitignore
branch
3 times, most recently
from
7ed05a4
to
610ea22
Compare
jbedard
force-pushed
the
gitignore
branch
9 times, most recently
from
698d1b1
to
52d7986
Compare
| name = "rename_%s_%s" % (TESTS[i], j), | ||
| srcs = [TEST_GEN_FILES_IN[i][j]], | ||
| outs = [TEST_GEN_FILES_OUT[i][j]], | ||
| cmd = "cp $< $@", |
There was a problem hiding this comment.
Something here is not working on windows where this genrule is either not being generated or is not dropping the .test- from the path, I assume because of slashes.
Anyone see anything wrong here? Or have a windows box to debug it more?
There was a problem hiding this comment.
I gave up and ignored this test on windows for now. I confirmed it is renaming the files but the .test-* files are still visible. I assume this is a windows+sandboxing issue?
jbedard
force-pushed
the
gitignore
branch
9 times, most recently
from
6b6c021
to
4886ea6
Compare
| } | ||
| } | ||
|
|
||
| shouldUpdate := updateRels.shouldUpdate(rel, updateParent) | ||
| for _, sub := range subdirs { | ||
| if subRel := path.Join(rel, sub); updateRels.shouldVisit(subRel, shouldUpdate) { | ||
| visit(c, cexts, isBazelIgnored, knownDirectives, updateRels, wf, filepath.Join(dir, sub), subRel, shouldUpdate) | ||
| // Create a new slice to allow the nested 'visit' call to modify the slice without affecting the parent. |
There was a problem hiding this comment.
do we expect the existing elements to get mutated? Or only append new ones? Asking because if relParts has capacity for an extra element, then subParts will be a new slice but they will share the same backing store, so mutating it's 0..n-1 index elements will also mutate relParts
There was a problem hiding this comment.
The only mutation is of entParts which is local to this method, however entParts = append(relParts, ".").
We might have to think about this more when the loop becomes concurrent though.
jbedard
force-pushed
the
gitignore
branch
3 times, most recently
from
ed09b76
to
0f82486
Compare
This reduces the noise a lot when adding more libraries such as in #1908 **What type of PR is this?** > Other **What package or component does this PR mostly affect?** > all **What does this PR do? Why is it needed?** **Which issues(s) does this PR fix?** Fixes # **Other notes for review**
What type of PR is this?
What package or component does this PR mostly affect?
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
.bazelignorebut 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