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

Dont traverse gitignored dirs for gitignore files #797

Conversation

robramsaynz
Copy link
Contributor

Updated version of #397.

this seems to be an upstream issue in go-git, and i'll prepare a PR for them soon, but for now this copies in the affected function and fixes it by checking the accumulated patterns while walking the fs looking for gitignore files

fixes: #389

robotdana and others added 11 commits February 2, 2024 11:47
this seems to be an upstream issue in go-git, and i'll prepare a PR for
them soon, but for now this copies in the affected method and fixes it
by checking the accumulated patterns while walking the fs looking for
gitignore files

fixes: google#389
Using reflect to test for a non-included patterns
- Fatalf() -> Errorf(): show all failures not just first
- adjust how the directory mid-tree dir is target in
  TestGitignoreFilesFromMidTree, I'm paranoid
  having fs point to root dir was giving false positives
Copy link

google-cla bot commented Feb 13, 2024

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

@robramsaynz robramsaynz marked this pull request as draft February 13, 2024 02:21
@robramsaynz robramsaynz marked this pull request as ready for review February 27, 2024 01:53
@robramsaynz
Copy link
Contributor Author

@another-rex This is ready to look at now. I may add a few more tests to it, but it currently works, and has reasonable test coverage. I'm not sure how to add you as a reviewer so I'm just @-ing you here.

Co-authored-by: Rex P <106129829+another-rex@users.noreply.github.com>
@robramsaynz
Copy link
Contributor Author

@another-rex RE:

Also, how different is this from upstream? I think ideally we push this change upstream, and avoid having to maintain this. But since it's only a few files, I'm not too concerned if we can't get it upstream.

I've moved things round to make them easier to diff, but it shouldn't be massively different. You can see the difference with:

I've tried to keep these fairly similar to make diffing easier. You can check the diff with:

$ curl https://raw.githubusercontent.com/go-git/go-git/v5.7.0/plumbing/format/gitignore/dir.go > a.go
$ curl https://raw.githubusercontent.com/ackama/osv-scanner/dont-traverse-gitignored-dirs-for-gitignore-files/pkg/osvscanner/customgitignore/dir.go > b.go
$ diff -u a.go b.go

We are messing with a parsing algorithm, so that's risky a little, but also have fairly good tests for this generally. I'm expecting upstream to like the changes. My guess is they wont want walk_up_to_root.go given they're a general purpose library and this is the "make it work like ripgrep" file. For instance the existing API of ReadPatterns() seems to assume it gets passed the root path of a git repo.

@robramsaynz robramsaynz requested a review from G-Rath March 3, 2024 23:19
internal/customgitignore/dir_test.go Outdated Show resolved Hide resolved
internal/customgitignore/dir_test.go Outdated Show resolved Hide resolved
internal/customgitignore/dir_test.go Outdated Show resolved Hide resolved
internal/customgitignore/dir_test.go Outdated Show resolved Hide resolved
internal/customgitignore/walk_up_to_root.go Outdated Show resolved Hide resolved
robramsaynz and others added 5 commits March 4, 2024 16:21
Co-authored-by: Gareth Jones <Jones258@Gmail.com>
Co-authored-by: Gareth Jones <Jones258@Gmail.com>
Co-authored-by: Gareth Jones <Jones258@Gmail.com>
Co-authored-by: Gareth Jones <Jones258@Gmail.com>
Copy link
Collaborator

@another-rex another-rex left a comment

Choose a reason for hiding this comment

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

LGTM!

@robramsaynz robramsaynz requested a review from G-Rath March 4, 2024 03:59
@codecov-commenter
Copy link

codecov-commenter commented Mar 4, 2024

Codecov Report

Attention: Patch coverage is 71.83908% with 49 lines in your changes are missing coverage. Please review.

Project coverage is 59.86%. Comparing base (b28c1c8) to head (f38dff0).
Report is 3 commits behind head on main.

Files Patch % Lines
internal/customgitignore/walk_up_to_root.go 72.13% 24 Missing and 10 partials ⚠️
internal/customgitignore/dir.go 68.75% 12 Missing and 3 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #797      +/-   ##
==========================================
+ Coverage   59.78%   59.86%   +0.07%     
==========================================
  Files         136      138       +2     
  Lines       11268    11424     +156     
==========================================
+ Hits         6737     6839     +102     
- Misses       4102     4145      +43     
- Partials      429      440      +11     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

another-rex pushed a commit that referenced this pull request Mar 4, 2024
We've already got this duplicated in two places, and I have need for it
in an upcoming PR and @robramsaynz should be using it in #797 so let's
move this into `testutility`
Comment on lines +646 to +647
// TODO: convert `t.TempDir()` to `testutility.CreateTestDir(t)` once PR #832 lands
dir := filepath.Join(t.TempDir(), "base_dir")
Copy link
Collaborator

Choose a reason for hiding this comment

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

#832 has been merged so this can be actioned now, though I don't think it should block landing this

@another-rex another-rex merged commit f7d28df into google:main Mar 4, 2024
11 checks passed
@G-Rath G-Rath deleted the dont-traverse-gitignored-dirs-for-gitignore-files branch March 4, 2024 23:17
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.

Gitignore parsing iterates through all directories unnecessarily
5 participants