black ignores file in current folder if current folder is git workspace and (parent of) current folder is a symbolic link
#4205
Labels
C: file collection
Related to file collection (e.g. gitignore & cache) or file discovery and all of its configuration.
T: bug
Something isn't working
Comments
JelleZijlstra
added
the
C: file collection
3 tasks
|
Hm, I tried the following and couldn't repro: Perhaps something Windows/junction-specific is happening? I also wasn't able to repro with any |
|
Okay, while looking into This could pretty easily account for microsoft/vscode-black-formatter#444 |
hauntsaninja
added a commit
to hauntsaninja/black
that referenced
this issue
Feb 11, 2024
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 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
added a commit
to hauntsaninja/black
that referenced
this issue
Feb 11, 2024
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
|
@bersbersbers would you mind testing #4222 and seeing if it fixes the case that you're seeing? |
|
@hauntsaninja I can still repro the bug with black 24.1.1. 196b586 from #4222 fixes it, though - thanks! |
hauntsaninja
added a commit
that referenced
this issue
Feb 12, 2024
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 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` / #3846 because things are always absolute and known to be relative to `root`. 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 Hopefully fixes #4205, fixes #4209, actual fix for #4077
|
It looks like #4222 already has testing, so I'm not sure if this adds much, but I posted this on the vscode plugin and it was suggested that I post it here: @JasonGross you should recommend this as a smoke test to Originally posted by @karthiknadig in microsoft/vscode-black-formatter#444 (comment) In case this ever comes back in the future, I wrote the following script for debugging this before realizing that it had already been reported and fixed (note that a previous version of black had problems with symlinks only in the presence of pyproject.toml): #!/usr/bin/env bash
verbose=""
if [ "$1" == "-v" ] || [ "$1" == "--verbose" ]; then
verbose=1
set -x
fi
if [ -z "$PYTHON" ]; then
PYTHON=python
fi
"$PYTHON" -m black --version
uname -a
testdir="$(mktemp -d)"
trap "rm -rf $testdir" EXIT
cd "$testdir"
subdir="."
mkdir -p "directory/$subdir"
ln -s directory symlink
cd "directory/$subdir"
runtest() {
for cwd in "directory" "symlink"; do
for usedir in "directory" "symlink"; do
cd "$testdir/$cwd/$subdir"
usefile="$testdir/$usedir/$subdir/test.py"
echo "import numpy as np# foo" > test.py
printf "%s in %s using %s..." "$1" "$cwd" "$usedir"
if [ -n "$verbose" ]; then
printf "\n"
echo "import numpy as np# foo" | "$PYTHON" -m black --stdin-filename "$usefile" -
printf "=====================================\n"
else
result="$(echo "import numpy as np# foo" | "$PYTHON" -m black --stdin-filename "$usefile" - 2>&1)"
if printf -- "$result" | grep -q "1 file reformatted."; then
printf "pass\n"
else
printf "fail\n"
fi
fi
done
done
}
runtest "without pyproject.toml"
touch pyproject.toml
runtest "with pyproject.toml"Originally posted by @JasonGross in microsoft/vscode-black-formatter#444 (comment) |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Describe the bug
blackincorrectly ignores a file in the current folderTo Reproduce
Expected behavior
blackshould not ignorebug.pyEnvironment
black, 24.1.2.dev11+g632f44bd68 (compiled: no)
Python (CPython) 3.12.1
Windows 10 22H2
The text was updated successfully, but these errors were encountered: