black icon indicating copy to clipboard operation
black copied to clipboard

black does not exclude files in git submodules

Open janjachnik-dyson opened this issue 2 years ago • 9 comments

Describe the bug

black does not exclude files in git submodules when full filename is passed

To Reproduce

I created an example project with a git submodule to test. The super-project has a pyproject.toml which excludes the subproject:

[tool.black]
force-exclude = 'subproject'

Commands to clone and reproduce bug:

git clone --recurse-submodules [email protected]:janjachnik-dyson/black-submodule-test-super.git
cd black-submodule-test-super
black --check subproject/file.py

The output:

Identified `/tmp/black-submodule-test-super/subproject` as project root containing a .git directory.
Sources to be formatted: "file.py"
subproject/file.py already well formatted, good job.

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

Expected behavior

subproject/file.py would be ignored because its path matches the force-exclude regex in pyproject.toml

Environment

  • Black's version: 22.3.0
  • OS and Python version: Ubuntu 20.04, Python 3.8.10

Additional context

We use black from within Visual Studio Code, where it is always called with the full filename. If black is manually run on the root folder of the super-project, the excludes are handled correctly. Within VSCode we pass the --config argument to specify the correct configuration file of the super-project.

Possible solution

The problem occurs because black decides the project root by recursively searching parent directories for .git, .hg and pyproject.toml files. In this case, the project root becomes the subproject folder (due to .git) and the exclude regex is compared against the path relative to the project root, which no longer contains the subproject string.

As a possible fix, I added the option to manually specify the project root: https://github.com/janjachnik-dyson/black/commit/6fc33683f17ecbb689add9c722ba96be0428d78c

Then, the following command works to properly exclude the file:

black --project-root . subproject/file.py

If this sounds like a valid fix/workaround, I'll raise a PR. Or is there a better way to handle excludes with git submodules?

janjachnik-dyson avatar Apr 28 '22 14:04 janjachnik-dyson

We've also been seeing issues where black doesn't correctly handle .gitignore with submodules. In our case it's a top-level .gitignore being incorrectly applied to files in submodules, eg.

$ black -v .
Identified `...` as project root containing a .git directory.
Sources to be formatted: "."
example.py ignored: matches the .gitignore file content
submodule/example.py ignored: matches the .gitignore file content

It would be nice for black to match git behaviour 1:1 by default, without needing CLI workarounds.

edwardpeek-crown avatar Jun 14 '22 04:06 edwardpeek-crown

I've been trying to write a test for this and I'm concerned that the existing exclude tests do not reflect real-world usage.

The existing tests make use of the assert_collected_sources() function: https://github.com/psf/black/blob/6c1bd08f16b636de38b92aeb2e0a1e8ebef0a0b1/tests/test_black.py#L1836 which in-term makes a call to get_sources(): https://github.com/psf/black/blob/6c1bd08f16b636de38b92aeb2e0a1e8ebef0a0b1/src/black/init.py#L595

However, the result of get_sources() is dependent on ctx.obj["root"]. That root folder is set dynamically when you run black using the find_project_root() function: https://github.com/psf/black/blob/6c1bd08f16b636de38b92aeb2e0a1e8ebef0a0b1/src/black/init.py#L458-L459

However, in all of the include/exclude tests the root folder is manually set. None of the tests will reproduce the behaviour that occurs if find_project_root() finds a different folder, which is the behaviour when you actually use black.

janjachnik-dyson avatar Jun 16 '22 15:06 janjachnik-dyson

In my branch, I have added a test which reproduces the behaviour of this bug: https://github.com/janjachnik-dyson/black/tree/feature/manually_specify_project_root

The problem only seems to occur when the full file path is given, I added tests for both scenarios (base path and full path).

To make sure the tests are run correctly, I now set ctx.obj['root'] inside assert_collected_sources() by making a call to black.find_project_root() - this mimics the behaviour of the black executable. Without that change, the test would pass.

So now we have the bug reproduced in a test, I'd like some feedback/guidance on how to fix it.

I've suggested one solution in a draft PR here: https://github.com/psf/black/pull/3116, which essentially gives the ability to manually override the project root.

Is my solution a viable option, or should the bug be fixed in a way that doesn't require manual intervention?

janjachnik-dyson avatar Jun 23 '22 10:06 janjachnik-dyson

I was expecting that black would never try to format files in any submodule, but maybe our team has a different setup than other python project. We have googletest as a submodule (i think is the normal way to use gtest), which seems to have python2 scripts which makes black fail. I could file a different feature-request for "black option to exclude files in git submodules"? (Or would that be a dupe of this issue?)

darthwalsh avatar Dec 07 '22 19:12 darthwalsh

Any news on this front? This is a major nuisance for us, would be great if a workaround was available.

derpda avatar Jan 12 '23 08:01 derpda

@derpda No update currently. I'm open to specifying the project root. I'll try to take a look at the linked PR soon-ish. A admittedly hacky workaround is to create an empty file at the root of your project and pass that file along with the file from the submodule.

black empty.py submodule/yourcode.py

This way Black will be forced to use the common parent of the two files as the project root.

ichard26 avatar Jan 15 '23 04:01 ichard26

Thank you for getting back to me on this! I tried this in VSCode, and it will sadly still format files that are in the force-exclude list. Weird thing is though, if I copy the command from VSCode's Ouput window and execute it in the shell it works as expected, ignoring the second file if it is in the exclude list. When formatting on save however, it will apply changes.

That is probably beyond the scope of this issue, but if you (or someone else) knows something, that would be much appreciated!

derpda avatar Jan 17 '23 01:01 derpda

Still relevant.

huntedman avatar Feb 21 '24 05:02 huntedman