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

porcelain.status: skip traversal of gitignore untracked dirs #853

Merged
merged 7 commits into from
Apr 7, 2021

Conversation

pmrowla
Copy link
Contributor

@pmrowla pmrowla commented Mar 17, 2021

When using porcelain.get_untracked_paths with exclude_ignored, currently every untracked path is checked to see if it is gitignored. If a directory is gitignored, we can skip checking the remainder of the files inside that directory - from the documentation for gitignore:

Git doesn’t list excluded directories for performance reasons, so any patterns on contained files have no effect, no matter where they are defined.

Changes after this PR:

  • porcelain.get_untracked_files: no longer walks ignored directories for performance reasons. The path to the ignored dir (including trailing slash) will be yielded if exclude_ignored=False, but individual files within the directory will not be included.
  • porcelain.add: now only returns path to ignored directory in the (added, ignored) tuple (previously all files inside ignored dirs would be returned)
  • porcelain.status: now only returns path to ignored directory when called with ignored=True (previously all files inside ignored dirs would be returned)

@codecov
Copy link

codecov bot commented Mar 17, 2021

Codecov Report

Merging #853 (24ec0da) into master (c60b83b) will increase coverage by 0.01%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #853      +/-   ##
==========================================
+ Coverage   84.67%   84.69%   +0.01%     
==========================================
  Files          90       90              
  Lines       22019    22047      +28     
  Branches     2354     2358       +4     
==========================================
+ Hits        18645    18673      +28     
  Misses       2961     2961              
  Partials      413      413              
Impacted Files Coverage Δ
dulwich/porcelain.py 81.01% <100.00%> (+0.29%) ⬆️
dulwich/tests/test_porcelain.py 99.91% <100.00%> (+<0.01%) ⬆️

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 c60b83b...24ec0da. Read the comment docs.

@pmrowla
Copy link
Contributor Author

pmrowla commented Mar 17, 2021

porcelain.add could probably also benefit from this?

Although the current behavior is to return the tuple of (added, ignored) where ignored contains all ignored paths, so it would need to be changed to return ignored directory paths instead of the files inside the directory

@jelmer
Copy link
Owner

jelmer commented Mar 17, 2021

The changes look good - any chance you could add a test to make sure this doesn't regress?

@jelmer
Copy link
Owner

jelmer commented Mar 17, 2021

I think it would make sense to change the behaviour of add as well - we should make sure to document the change in behaviour in NEWS.

@pmrowla pmrowla force-pushed the status-ignored-dir branch from f601f65 to e336d39 Compare March 18, 2021 10:26
"ignored",
os.path.join("subdir", ""),
]
),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This test should catch any future regression, if we recurse into the ignored subdir here, we will end up yielding subdir/ignored as well.

@pmrowla pmrowla force-pushed the status-ignored-dir branch from e336d39 to 24ec0da Compare April 7, 2021 09:09
@pmrowla
Copy link
Contributor Author

pmrowla commented Apr 7, 2021

@jelmer this is also ready for review

@jelmer jelmer merged commit d7c89e1 into jelmer:master Apr 7, 2021
@pmrowla pmrowla deleted the status-ignored-dir branch April 8, 2021 00:57
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.

2 participants