Output incorrectly claims symbolic links presence when formatting from outside a project

[aaossa]

Describe the bug

When trying to format a project from the outside, the verbose output shows says that there are symbolic links that points outside of the project, but displays the wrong project path. This behavior seem to be triggered when the command is executed from outside a project on a folder.

To Reproduce

Consider the following tree:

.
└── home/
    └── project/
        ├── .git
        └── dir/
            └── main.py

When trying to format a folder from home, this is the output:

$ black ./project --check --verbose
Identified `/path/to/home/project` as project root containing a .git directory.
Sources to be formatted: "."
project/.git ignored: is a symbolic link that points outside /path/to/home/project/project
project/.git ignored: matches the --exclude regular expression
project/dir ignored: is a symbolic link that points outside /path/to/home/project/project
project/dir/file.py ignored: is a symbolic link that points outside /path/to/home/project/project
would reformat project/dir/file.py

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

Notice that the messageFILEPATH ignored: is a symbolic link that points outside PROJECTPATH displays a wrong PROJECTPATH (should be /path/to/home/project/ instead of /path/to/home/project/project).

Same happens when using black ./project/dir:

$ black ./project/dir --check --verbose
Identified `/path/to/home/project` as project root containing a .git directory.
Sources to be formatted: "dir"
project/dir/file.py ignored: is a symbolic link that points outside /path/to/home/project/project/dir       
would reformat project/dir/file.py

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

Expected behavior

Display each path once (and correctly):

$ black ./project/ --check --verbose
Identified `/path/to/home/project` as project root containing a .git directory.
Sources to be formatted: "."
/path/to/home/project/.git ignored: matches the --exclude regular expression
would reformat /path/to/home/project/dir/file.py

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

$ black ./project/dir/ --check --verbose
Identified `/path/to/home/project` as project root containing a .git directory.
Sources to be formatted: "dir"
would reformat /path/to/home/project/dir/file.py  

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

Environment

• Black's version: main
• OS and Python version: Linux/Python 3.8.10

Additional context

This change produces the expected behavior:

diff --git a/src/black/__init__.py b/src/black/__init__.py
index 7d7ddbe..2897c5b 100644
--- a/src/black/__init__.py
+++ b/src/black/__init__.py
@@ -666,10 +666,11 @@ def get_sources(

             sources.add(p)
         elif p.is_dir():
+            p = root / normalize_path_maybe_ignore(p, ctx.obj["root"], report)
             if using_default_exclude:
                 gitignore = {
                     root: root_gitignore,
-                    root / p: get_gitignore(p),
+                    p: get_gitignore(p),
                 }
             sources.update(
                 gen_python_files(

I'll try to submit a PR. I'm trying to figure out how to test this.

[aaossa]

Seems like this can't be tested easily because the exception can't be reproduced in the test suite:

black.get_sources(
    ctx=ctx,
    src=("./dir",),
    quiet=False,
    verbose=True,
    include=DEFAULT_INCLUDE,
    exclude=DEFAULT_EXCLUDE,
    report=report,
    extend_exclude=None,
    force_exclude=None,
    stdin_filename=None,
)

This function call is similar to the call from the CLI, but the test suite fails because src is an invalid value (is_dir() returns false), because every test uses assert_collected_sources and passes an absolute path to the source/target files. But, in reality, the CLI receives relative paths. This means that the test suite is not reproducing the CLI behavior 100% correctly. This seems like another issue, related to #3040 (comment) but not the same.

This reported bug does not happen in the test, because (as you can see in the patch) the solution uses the absolute path to the target file.

[aaossa]

Here's a working test. Current behavior makes the test fail (call_count is 4) while patch makes the tesst pass (call_count is 3). Had to patch some functions because of the differences between the actual behavior when using the CLI (using relative paths) and the tested behavior (which uses absolute paths):

diff --git a/tests/test_black.py b/tests/test_black.py
index dda1055..07aa8b3 100644
--- a/tests/test_black.py
+++ b/tests/test_black.py
@@ -475,6 +475,35 @@ class BlackTestCase(BlackBaseTestCase):
         self.assertFormatEqual(contents_spc, fs(contents_spc))
         self.assertFormatEqual(contents_spc, fs(contents_tab))

+    @patch("pathlib.Path.is_dir", lambda _: True)
+    def test_false_positive_symlink_output_issue_3384(self) -> None:
+        # Emulate the behavior when using the CLI: black ./child  --check --verbose
+        project_root = Path(THIS_DIR / "data" / "nested_gitignore_tests")
+        working_directory = project_root / "root"
+        target_abspath = working_directory / "child"
+        target_contents = (
+            src.relative_to(working_directory) for src in target_abspath.iterdir()
+        )
+        with patch("pathlib.Path.iterdir", return_value=target_contents), patch(
+            "pathlib.Path.cwd", return_value=working_directory
+        ):
+            ctx = FakeContext()
+            ctx.obj["root"] = project_root
+            report = MagicMock(verbose=True)
+            collected = black.get_sources(
+                ctx=ctx,
+                src=("./child",),
+                quiet=False,
+                verbose=True,
+                include=DEFAULT_INCLUDE,
+                exclude=None,
+                report=report,
+                extend_exclude=None,
+                force_exclude=None,
+                stdin_filename=None,
+            )
+            assert report.path_ignored.call_count == 3
+
     def test_report_verbose(self) -> None:
         report = Report(verbose=True)
         out_lines = []

[aaossa]

Hi, is there any updates on this?