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

black could more efficiently skip directories listed in .gitignore #4405

Closed
rmcloughlin opened this issue Jul 18, 2024 · 2 comments · Fixed by #4415
Closed

black could more efficiently skip directories listed in .gitignore #4405

rmcloughlin opened this issue Jul 18, 2024 · 2 comments · Fixed by #4415
Labels
T: bug Something isn't working

Comments

@rmcloughlin
Copy link

rmcloughlin commented Jul 18, 2024

Say I have this file structure:

my-project
├── large_ignored_dir
│   ├── a.py
│   ├── inner
│   │   └── b.py
│   ├── inner2
│   │   └── c.py
│   └── inner3
│       └── d.py
└── z.py

The only file I want black to pay attention to is z.py.

Then I run black --exclude='large_ignored_dir/' --check -v . which shows me:

Identified `/` as project root containing a file system root.
Found input source directory: "[...]/my-project"
[...]/my-project/large_ignored_dir ignored: matches the --exclude regular expression
would reformat [...]/my-project/z.py

Oh no! 💥 💔 💥
1 file would be reformatted.

That seems to be the correct behavior.

But black is supposed to automatically ignore directories listed in .gitignore (docs). So if I add a .gitignore file containing this line:

large_ignored_dir/

Then I should be able to run black --check -v . and see the same result as above. But instead I see:

Identified `/` as project root containing a file system root.
Found input source directory: "[...]/my-project"
[...]/my-project/large_ignored_dir/inner ignored: matches a .gitignore file content
[...]/my-project/large_ignored_dir/a.py ignored: matches a .gitignore file content
[...]/my-project/large_ignored_dir/inner2 ignored: matches a .gitignore file content
[...]/my-project/large_ignored_dir/inner3 ignored: matches a .gitignore file content
would reformat [...]/my-project/z.py

Oh no! 💥 💔 💥
1 file would be reformatted.

Even though the final result is the same (only z.py is reformatted), in this second case black is wasting time checking files that it could have skipped. Instead of ignoring all of large_ignored_dir it is descending one level inside it and then deciding to skip every file and subdirectory.

This has performance consequences with directories like node_modules and __pycache__.

Environment

  • Black's version: 24.4.2
  • OS and Python version: macOS Sonoma 14.5, M1 chip, Python 3.12
@rmcloughlin rmcloughlin added the T: bug Something isn't working label Jul 18, 2024
@JelleZijlstra
Copy link
Collaborator

PR welcome to improve this.

@devdanzin
Copy link

Interestingly, if the content of .gitignore is set to large_ignored_dir without a trailing slash, it works as expected:

Found input source directory: "[...]\my-project"
[...]\my-project\large_ignored_dir ignored: matches a .gitignore file content
[...]\my-project\z.py already well formatted, good job.

All done! ✨ 🍰 ✨
1 file would be left unchanged.

So maybe we should add a trailing slash to relative_path in _path_is_ignored() if the path is a directory?

black/src/black/files.py

Lines 301 to 316 in 721dff5

def _path_is_ignored(
root_relative_path: str,
root: Path,
gitignore_dict: Dict[Path, PathSpec],
) -> bool:
path = root / root_relative_path
# Note that this logic is sensitive to the ordering of gitignore_dict. Callers must
# ensure that gitignore_dict is ordered from least specific to most specific.
for gitignore_path, pattern in gitignore_dict.items():
try:
relative_path = path.relative_to(gitignore_path).as_posix()
except ValueError:
break
if pattern.match_file(relative_path):
return True
return False

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T: bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants