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

Fewer dirs open #47

Closed
wants to merge 4 commits into from
Closed

Fewer dirs open #47

wants to merge 4 commits into from

Conversation

luhring
Copy link
Contributor

@luhring luhring commented Oct 19, 2020

Hello! This PR does the following:

  1. It introduces a minimal amount of benchmarking. I came across issue any benchmark #19 after noticing some slowness in the implementation (for how we're using it specifically). There's certainly more that can be done with benckmarks, but I see from reading the issue that performance isn't a key criterion for doublestar — so feel free to discard this part!
  2. It introduces an executable entrypoint for the Glob function, just so that it's easy to examine behavior of the function quickly from the shell, without needing to incorporate the function into a larger project. For example, you can run go run ./examples/find.go '/Users/dan/**/*.go'.
  3. It addresses an issue where, as directories were being traversed, directories remain open even after the contained files had been read. Every opened directory was ultimately closed, but because the call to dir.Close() was deferred, and the function body was large enough that recursive operations were being performed within the same function, directories ended up remaining open longer than necessary.

Signed-off-by: Dan Luhring <dan.luhring@anchore.com>
Signed-off-by: Dan Luhring <dan.luhring@anchore.com>
Signed-off-by: Dan Luhring <dan.luhring@anchore.com>
Signed-off-by: Dan Luhring <dan.luhring@anchore.com>
@codecov
Copy link

codecov bot commented Oct 19, 2020

Codecov Report

Merging #47 into master will decrease coverage by 0.93%.
The diff coverage is 63.63%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #47      +/-   ##
==========================================
- Coverage   80.55%   79.62%   -0.94%     
==========================================
  Files           1        1              
  Lines         432      319     -113     
==========================================
- Hits          348      254      -94     
+ Misses         60       40      -20     
- Partials       24       25       +1     
Impacted Files Coverage Δ
doublestar.go 79.62% <63.63%> (-0.94%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c4b4ad9...77f44b5. Read the comment docs.

@bmatcuk
Copy link
Owner

bmatcuk commented Oct 24, 2020

Hi @luhring! Thanks for the PR! Makes sense to me... I appreciate that you broke all the changes into individual commits, too! I merged in your changes to close directories, and your example program. I left out the changes to gitignore and your benchmarks - but only because I have a branch I've been working on that has benchmarks. I also back-ported your code to the v1 branch =)

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

Successfully merging this pull request may close these issues.

None yet

2 participants