Fix ignoring input files for symlink reasons #4222

hauntsaninja · 2024-02-11T08:05:32Z

This relates to #4015, #4161 and the behaviour of os.getcwd()

Black is a big user of pathlib and as such loves doing .resolve(), since for a long time it was the only good way of getting an absolute path in pathlib. However, this has two problems:

The first minor problem is performance, e.g. in #3751 I (safely) got rid of a bunch of .resolve() which made Black 40% faster on cached runs.

The second more important problem is that always resolving symlinks results in unintuitive exclusion behaviour. For instance, a gitignored symlink should never alter formatting of your actual code. This kind of thing was reported by users a few times.

In #3846, I improved the exclusion rule logic for symlinks in gen_python_files and everything was good.

But gen_python_files isn't enough, there's also get_sources, which handles user specified paths directly (instead of files Black discovers). So in #4015, I made a very similar change to #3846 for get_sources, and this is where some problems began.

The core issue was the line:

root_relative_path = path.absolute().relative_to(root).as_posix()

The first issue is that despite root being computed from user inputs, we call .resolve() while computing root (likely unecessarily). Which means that path may not actually be relative to root. So I started off this PR trying to fix that, when I ran into the second issue. Which is that os.getcwd() (as called by os.path.abspath or Path.absolute or Path.cwd) also often resolves symlinks!

>>> import os
>>> os.environ.get("PWD")
'/Users/shantanu/dev/black/symlink/bug'
>>> os.getcwd()
'/Users/shantanu/dev/black/actual/bug'

This also meant that the breakage often would not show up when input relative paths.

This doesn't affect gen_python_files / #3846 because things are known to be relative to root and always absolute.

Anyway, it looks like #4161 fixed the crash by just swallowing the error and ignoring the file. Instead, we should just try to compute the actual relative path. I think this PR should be quite safe, but we could also consider reverting some of the previous changes; the associated issues weren't too popular.

At the same time, I think there's still behaviour that can be improved and I kind of want to make larger changes, but maybe I'll save that for if we do something like #3952

Fixes #4205, fixes #4209, original report in #4077

This relates to psf#4015, psf#4161 and the behaviour of os.getcwd() Black is a big user of pathlib and as such loves doing `.resolve()`, since for a long time it was the only good way of getting an absolute path in pathlib. However, this has two problems: The first minor problem is performance, e.g. in psf#3751 I (safely) got rid of a bunch of `.resolve()` which made Black 40% faster on cached runs. The second more important problem is that always resolving symlinks results in unintuitive exclusion behaviour. For instance, a gitignored symlink should never alter formatting of your actual code. This kind of thing was reported by users a few times. In psf#3846, I improved the exclusion rule logic for symlinks in `gen_python_files` and everything was good. But `gen_python_files` isn't enough, there's also `get_sources`, which handles user specified paths directly (instead of files Black discovers). So in psf#4015, I made a very similar change to psf#3846 for `get_sources`, and this is where some problems began. The core issue was the line: ``` root_relative_path = path.absolute().relative_to(root).as_posix() ``` The first issue is that despite root being computed from user inputs, we call `.resolve()` while computing it (likely unecessarily). Which means that `path` may not actually be relative to `root`. So I started off this PR trying to fix that, when I ran into the second issue. Which is that `os.getcwd()` (as called by `os.path.abspath` or `Path.absolute` or `Path.cwd`) also often resolves symlinks! ``` >>> import os >>> os.environ.get("PWD") '/Users/shantanu/dev/black/symlink/bug' >>> os.getcwd() '/Users/shantanu/dev/black/actual/bug' ``` This also meant that the breakage often would not show up when input relative paths. This doesn't affect `gen_python_files` / psf#3846 because things are always absolute and known to be relative to `root`. Anyway, it looks like psf#4161 fixed the crash by just swallowing the error and ignoring the file. Instead, we should just try to compute the actual relative path. I think this PR should be quite safe, but we could also consider reverting some of the previous changes; the associated issues weren't too popular. At the same time, I think there's still behaviour that can be improved and I kind of want to make larger changes, but maybe I'll save that for if we do something like psf#3952 Hopefully fixes psf#4205, fixes psf#4209, actual fix for psf#4077

hauntsaninja · 2024-02-11T08:27:50Z

src/black/__init__.py

@@ -734,6 +734,7 @@ def get_sources(
    """Compute the set of files to be formatted."""
    sources: Set[Path] = set()

+    assert root.is_absolute(), f"INTERNAL ERROR: `root` must be absolute but is {root}"


We have a similar assert in gen_python_files (which this usually calls)

hauntsaninja · 2024-02-11T08:29:24Z

src/black/__init__.py

-            root_relative_path = get_root_relative_path(path, root, report)
-
-            if root_relative_path is None:
+            if resolves_outside_root_or_cannot_stat(path, root, report):


As you may have guessed from the name, the difference between this and get_root_relative_path (from #4161) is the .resolve()

This helped me located where the filtering happens and I just added sources.add(path) before the old "continue"in line 755/756 and my problem was solved.
Tho this, if you need, I can still test it against my case where my home folder ~ was set to a symlink like /homes/user -> /external/homes/user. Please don't hesitate to tell me if you need any help! Thanks for the great work again!

hauntsaninja · 2024-02-11T08:32:03Z

src/black/files.py

+        return path.absolute().relative_to(root)
+    except ValueError:
+        pass
+    root_parent = next((p for p in path.parents if _cached_resolve(p) == root), None)


I added _cached_resolve mainly in case this loop x many input files is expensive

hauntsaninja · 2024-02-11T08:32:32Z

src/black/files.py

@@ -339,7 +348,8 @@ def gen_python_files(

    assert root.is_absolute(), f"INTERNAL ERROR: `root` must be absolute but is {root}"
    for child in paths:
-        root_relative_path = child.absolute().relative_to(root).as_posix()
+        assert child.is_absolute()


Not new, this should be guaranteed by path = root / (path.resolve().relative_to(root)) in get_sources

github-actions · 2024-02-11T08:40:49Z

diff-shades reports zero changes comparing this PR (43aad90) to main (a201003).

What is this? | Workflow run | diff-shades documentation

hauntsaninja · 2024-02-11T08:58:34Z

tests/test_black.py

                src=["-"],
                expected=[],
                stdin_filename=stdin_filename,
            )

-    @patch("black.find_project_root", lambda *args: (THIS_DIR.resolve(), None))
    def test_get_sources_with_stdin(self) -> None:


Not really related, but while I was here I removed some monkeypatching since it's not needed. We probably do a little too much mocking, so the new tests touch the file system

Agree we should prefer to avoid mocking.

PabloLION · 2024-02-11T09:01:48Z

(fake@)hauntsaninja, Thank you for your continuous and beautiful work, I was waiting for this for a while, and I thought #4221 solves this problem. But sadly, no.
An idea: How about adding a work around flag like --experimental-force-ignore-symlink-check, since it's blocking many including me myself? If approved, I'll be glad to PR on this.

hauntsaninja · 2024-02-11T09:34:48Z

PabloLION, thanks for taking a look at this PR. Does this PR solve the thing blocking you? If it does, we can make a release relatively quickly once it's merged.

PabloLION · 2024-02-11T09:54:39Z

I replied between the code #4222 (comment)
please let me know if assist is needed.

JelleZijlstra · 2024-02-11T16:08:54Z

tests/test_black.py

    def test_get_sources_with_stdin_symlink_outside_root(
        self,
    ) -> None:
        path = THIS_DIR / "data" / "include_exclude_tests"
        stdin_filename = str(path / "b/exclude/a.py")
        outside_root_symlink = Path("/target_directory/a.py")
+        root = Path("target_dir/").resolve().absolute()


target_dir/ or target_directory?

target_dir is right. Rename doesn't affect semantics, I did it to make the test clearer, the relevant difference is /target_directory is absolute and so is outside root

I see, it would have been slightly clearer if you used two names that didn't look like one was a variant of the other, e.g. target_dir1 and target_dir2. What you have now is fine too though.

JelleZijlstra · 2024-02-11T16:09:11Z

tests/test_black.py

                src=["-"],
                expected=[],
                stdin_filename=stdin_filename,
            )

-    @patch("black.find_project_root", lambda *args: (THIS_DIR.resolve(), None))
    def test_get_sources_with_stdin(self) -> None:


Agree we should prefer to avoid mocking.

JelleZijlstra

Looks good, thanks. Some optional nits.

src/black/files.py

Co-authored-by: Jelle Zijlstra <[email protected]>

hauntsaninja added 2 commits February 11, 2024 00:10

annoying encoding test

ca4da79

fix for windows <= 3.9

196b586

hauntsaninja commented Feb 11, 2024

View reviewed changes

JelleZijlstra reviewed Feb 11, 2024

View reviewed changes

change

4c106e9

JelleZijlstra approved these changes Feb 11, 2024

View reviewed changes

src/black/files.py Outdated Show resolved Hide resolved

Update src/black/files.py

43aad90

Co-authored-by: Jelle Zijlstra <[email protected]>

hauntsaninja merged commit 23dfc5b into psf:main Feb 12, 2024
46 checks passed

hauntsaninja deleted the normalise-simplify branch February 12, 2024 08:04

reporter123 mentioned this pull request Feb 19, 2024

Actually use result of best_effort_relative_path #4242

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix ignoring input files for symlink reasons #4222

Fix ignoring input files for symlink reasons #4222

hauntsaninja commented Feb 11, 2024 •

edited

Loading

hauntsaninja Feb 11, 2024

hauntsaninja Feb 11, 2024

PabloLION Feb 11, 2024

hauntsaninja Feb 11, 2024 •

edited

Loading

hauntsaninja Feb 11, 2024

github-actions bot commented Feb 11, 2024 •

edited

Loading

hauntsaninja Feb 11, 2024

JelleZijlstra Feb 11, 2024

PabloLION commented Feb 11, 2024

hauntsaninja commented Feb 11, 2024

PabloLION commented Feb 11, 2024

JelleZijlstra Feb 11, 2024

hauntsaninja Feb 11, 2024

JelleZijlstra Feb 11, 2024

JelleZijlstra Feb 11, 2024

JelleZijlstra left a comment

Fix ignoring input files for symlink reasons #4222

Fix ignoring input files for symlink reasons #4222

Conversation

hauntsaninja commented Feb 11, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hauntsaninja Feb 11, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Feb 11, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PabloLION commented Feb 11, 2024

hauntsaninja commented Feb 11, 2024

PabloLION commented Feb 11, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JelleZijlstra left a comment

Choose a reason for hiding this comment

hauntsaninja commented Feb 11, 2024 •

edited

Loading

hauntsaninja Feb 11, 2024 •

edited

Loading

github-actions bot commented Feb 11, 2024 •

edited

Loading