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

feat: add support for .gitignore #1908

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

jbedard
Copy link
Contributor

@jbedard jbedard commented Sep 4, 2024

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

@jbedard
Copy link
Contributor Author

jbedard commented Sep 5, 2024

@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?

@tyler-french
Copy link
Contributor

How is this different from using 'gazelle:exclude' directives?

@jbedard
Copy link
Contributor Author

jbedard commented Sep 5, 2024

People won't want to copy+paste gitignore files into BUILD files...?

@jbedard
Copy link
Contributor Author

jbedard commented Sep 5, 2024

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 !...

@fmeum
Copy link
Member

fmeum commented Sep 5, 2024

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.

@jbedard
Copy link
Contributor Author

jbedard commented Sep 5, 2024

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.

@jbedard
Copy link
Contributor Author

jbedard commented Sep 5, 2024

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:

walk/config.go Outdated Show resolved Hide resolved
walk/config.go Outdated Show resolved Hide resolved
walk/config.go Outdated Show resolved Hide resolved
@jbedard jbedard force-pushed the gitignore branch 3 times, most recently from 7ed05a4 to 610ea22 Compare September 6, 2024 01:32
@jbedard jbedard requested a review from dzbarsky September 6, 2024 01:36
@jbedard jbedard force-pushed the gitignore branch 9 times, most recently from 698d1b1 to 52d7986 Compare September 6, 2024 08:53
name = "rename_%s_%s" % (TESTS[i], j),
srcs = [TEST_GEN_FILES_IN[i][j]],
outs = [TEST_GEN_FILES_OUT[i][j]],
cmd = "cp $< $@",
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

walk/walk.go Outdated Show resolved Hide resolved
walk/walk.go Outdated Show resolved Hide resolved
walk/walk.go Outdated Show resolved Hide resolved
walk/config.go Outdated Show resolved Hide resolved
fmeum pushed a commit that referenced this pull request Sep 16, 2024
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**
@DavidZbarsky-at
Copy link

This looks great to me, @fmeum wdyt?

@jbedard jbedard force-pushed the gitignore branch 3 times, most recently from 55f9844 to cce54f9 Compare September 21, 2024 01:42
entParts[len(entParts)-1] = base

// Skip git-ignored when the gitignore option is enabled.
if wc.isGitIgnored(entParts, ent.IsDir()) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note this is its own conditional and not embedded within isExcluded for a few reasons:

  • I don't think .gitignore can exclude the parent directory it is within the way # gazelle:exclude * can (that is the reason we must invoke isExcluded both above and within this for loop).
  • isGitIgnored requires the path as []string and needs to know IsDir(), I don't think we should override isExcluded with more params

jbedard added a commit to aspect-build/aspect-cli that referenced this pull request Sep 27, 2024
… gazelle feature implementation (#6873)

This aligns the implementation more with what I'm trying to merge into
gazelle: bazel-contrib/bazel-gazelle#1908

There are no user facing changes here but the implementation is simpler
and should improve performance for many reasons:
* the .gitignore files are only `os.Open`ed once per directory (not per
directory * per language)
* the .gitignore files are only parsed once instead of once per gazelle
language
* the .gitignore patterns are only executed once because it is done
within the (patched) gazelle walk
* .gitignore-d directories are not recursed into at all because they are
filtered in the gazelle fs walk
* ...

---

### Changes are visible to end-users: no

### Test plan

- Covered by existing test cases

GitOrigin-RevId: 27fe8377ca89de91d3c95105ab37adea9347e6ab
jbedard added a commit to aspect-build/aspect-cli that referenced this pull request Sep 28, 2024
… gazelle feature implementation (#6873)

This aligns the implementation more with what I'm trying to merge into
gazelle: bazel-contrib/bazel-gazelle#1908

There are no user facing changes here but the implementation is simpler
and should improve performance for many reasons:
* the .gitignore files are only `os.Open`ed once per directory (not per
directory * per language)
* the .gitignore files are only parsed once instead of once per gazelle
language
* the .gitignore patterns are only executed once because it is done
within the (patched) gazelle walk
* .gitignore-d directories are not recursed into at all because they are
filtered in the gazelle fs walk
* ...

---

### Changes are visible to end-users: no

### Test plan

- Covered by existing test cases

GitOrigin-RevId: 27fe8377ca89de91d3c95105ab37adea9347e6ab
jbedard added a commit to aspect-build/aspect-cli that referenced this pull request Sep 28, 2024
… gazelle feature implementation (#6873)

This aligns the implementation more with what I'm trying to merge into
gazelle: bazel-contrib/bazel-gazelle#1908

There are no user facing changes here but the implementation is simpler
and should improve performance for many reasons:
* the .gitignore files are only `os.Open`ed once per directory (not per
directory * per language)
* the .gitignore files are only parsed once instead of once per gazelle
language
* the .gitignore patterns are only executed once because it is done
within the (patched) gazelle walk
* .gitignore-d directories are not recursed into at all because they are
filtered in the gazelle fs walk
* ...

---

### Changes are visible to end-users: no

### Test plan

- Covered by existing test cases

GitOrigin-RevId: 27fe8377ca89de91d3c95105ab37adea9347e6ab
jbedard added a commit to aspect-build/aspect-cli that referenced this pull request Sep 28, 2024
… gazelle feature implementation (#6873)

This aligns the implementation more with what I'm trying to merge into
gazelle: bazel-contrib/bazel-gazelle#1908

There are no user facing changes here but the implementation is simpler
and should improve performance for many reasons:
* the .gitignore files are only `os.Open`ed once per directory (not per
directory * per language)
* the .gitignore files are only parsed once instead of once per gazelle
language
* the .gitignore patterns are only executed once because it is done
within the (patched) gazelle walk
* .gitignore-d directories are not recursed into at all because they are
filtered in the gazelle fs walk
* ...

---

### Changes are visible to end-users: no

### Test plan

- Covered by existing test cases

GitOrigin-RevId: 27fe8377ca89de91d3c95105ab37adea9347e6ab
@fmeum
Copy link
Member

fmeum commented Oct 23, 2024

Based on aspect-build/aspect-cli#755, 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?

@jbedard
Copy link
Contributor Author

jbedard commented Oct 23, 2024

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: #1166 (comment)

jbedard added a commit to aspect-build/aspect-cli that referenced this pull request Oct 23, 2024
Gitignore and generation-mode are both generic gazelle config and should require no code in plugins.

Note that both of these are currently patched into gazelle while waiting for PRs which
will allow the removal of all aspect code:
bazel-contrib/bazel-gazelle#1921
bazel-contrib/bazel-gazelle#1908

---

### Changes are visible to end-users: no

### Test plan

- Covered by existing test cases

GitOrigin-RevId: cdfe6690ff0ab2584e429419612cc32e94c6b379
jbedard added a commit to aspect-build/aspect-cli that referenced this pull request Oct 24, 2024
Gitignore and generation-mode are both generic gazelle config and should require no code in plugins.

Note that both of these are currently patched into gazelle while waiting for PRs which
will allow the removal of all aspect code:
bazel-contrib/bazel-gazelle#1921
bazel-contrib/bazel-gazelle#1908

---

### Changes are visible to end-users: no

### Test plan

- Covered by existing test cases

GitOrigin-RevId: cdfe6690ff0ab2584e429419612cc32e94c6b379
jbedard added a commit to aspect-build/aspect-cli that referenced this pull request Oct 24, 2024
Gitignore and generation-mode are both generic gazelle config and should require no code in plugins.

Note that both of these are currently patched into gazelle while waiting for PRs which
will allow the removal of all aspect code:
bazel-contrib/bazel-gazelle#1921
bazel-contrib/bazel-gazelle#1908

---

### Changes are visible to end-users: no

### Test plan

- Covered by existing test cases

GitOrigin-RevId: cdfe6690ff0ab2584e429419612cc32e94c6b379
jbedard added a commit to aspect-build/aspect-cli that referenced this pull request Oct 24, 2024
Gitignore and generation-mode are both generic gazelle config and should require no code in plugins.

Note that both of these are currently patched into gazelle while waiting for PRs which
will allow the removal of all aspect code:
bazel-contrib/bazel-gazelle#1921
bazel-contrib/bazel-gazelle#1908

---

### Changes are visible to end-users: no

### Test plan

- Covered by existing test cases

GitOrigin-RevId: cdfe6690ff0ab2584e429419612cc32e94c6b379
jbedard added a commit to aspect-build/aspect-cli that referenced this pull request Oct 24, 2024
Gitignore and generation-mode are both generic gazelle config and should require no code in plugins.

Note that both of these are currently patched into gazelle while waiting for PRs which
will allow the removal of all aspect code:
bazel-contrib/bazel-gazelle#1921
bazel-contrib/bazel-gazelle#1908

---

### Changes are visible to end-users: no

### Test plan

- Covered by existing test cases

GitOrigin-RevId: cdfe6690ff0ab2584e429419612cc32e94c6b379
jbedard added a commit to aspect-build/aspect-cli that referenced this pull request Oct 24, 2024
Gitignore and generation-mode are both generic gazelle config and should require no code in plugins.

Note that both of these are currently patched into gazelle while waiting for PRs which
will allow the removal of all aspect code:
bazel-contrib/bazel-gazelle#1921
bazel-contrib/bazel-gazelle#1908

---

### Changes are visible to end-users: no

### Test plan

- Covered by existing test cases

GitOrigin-RevId: cdfe6690ff0ab2584e429419612cc32e94c6b379
jbedard added a commit to aspect-build/aspect-cli that referenced this pull request Oct 24, 2024
Gitignore and generation-mode are both generic gazelle config and should require no code in plugins.

Note that both of these are currently patched into gazelle while waiting for PRs which
will allow the removal of all aspect code:
bazel-contrib/bazel-gazelle#1921
bazel-contrib/bazel-gazelle#1908

---

### Changes are visible to end-users: no

### Test plan

- Covered by existing test cases

GitOrigin-RevId: cdfe6690ff0ab2584e429419612cc32e94c6b379
@jbedard
Copy link
Contributor Author

jbedard commented Dec 3, 2024

Rebased. @fmeum any updates with getting this reviewed/merged?

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

Successfully merging this pull request may close these issues.

Exclude files/folders that are .gitignore'd
5 participants