Escape template filenames in glob patterns by ntBre · Pull Request #16407 · astral-sh/ruff

ntBre · 2025-02-26T22:17:19Z

Summary

Fixes #9381. This PR fixes errors like

Cause: error parsing glob '/Users/me/project/{{cookiecutter.project_dirname}}/__pycache__': nested alternate groups are not allowed

caused by glob special characters in filenames like {{cookiecutter.project_dirname}}. When the user is matching that directory exactly, they can use the workaround given by #7959 (comment), but that doesn't work for a nested config file with relative paths. For example, the directory tree in the reproduction repo linked here:

.
├── README.md
├── hello.py
├── pyproject.toml
├── uv.lock
└── {{cookiecutter.repo_name}}
    ├── main.py
    ├── pyproject.toml
    └── tests
        └── maintest.py

where the inner pyproject.toml contains a relative glob:

[tool.ruff.lint.per-file-ignores]
"tests/*" = ["F811"]

Test Plan

A new CLI test in both the linter and formatter. The formatter test may not be necessary because I didn't have to modify any additional code to pass it, but the original report mentioned both check and format, so I wanted to be sure both were fixed.

github-actions · 2025-02-26T22:23:38Z

`ruff-ecosystem` results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

✅ ecosystem check detected no linter changes.

Formatter (stable)

✅ ecosystem check detected no format changes.

Formatter (preview)

✅ ecosystem check detected no format changes.

ntBre · 2025-02-26T23:15:33Z

I'm not sure what's going wrong on Windows. It looks like the tempdir_filter isn't working. I force pushed back over it, but I even tried removing my second filter here and the temp path is still wrong.

MichaReiser · 2025-02-27T07:59:52Z

I'm not sure what's going wrong on Windows. It looks like the tempdir_filter isn't working. I force pushed back over it, but I even tried removing my second filter here and the temp path is still wrong.

Yeah, the filters are brutal on windows. Can you try what we have in

ruff/crates/red_knot/tests/cli.rs

Lines 863 to 865 in 3e840ca

    
           let mut settings = insta::Settings::clone_current(); 
        
           settings.add_filter(&tempdir_filter(&project_dir), "<temp_dir>/"); 
        
           settings.add_filter(r#"\\(\w\w|\s|\.|")"#, "/$1");

MichaReiser · 2025-02-27T08:02:04Z

+        Err(e) if *e.kind() == globset::ErrorKind::NestedAlternates => {
+            let new = s.replace("{{", "[{][{]").replace("}}", "[}][}]");
+            log::warn!(
+                "Error parsing original glob: `{s:?}`, trying with escaped braces: `{new:?}`"
+            );
+            Ok(Glob::new(&new)?)
+        }
+        glob => Ok(glob?),


This, indeed, feels somewhat hacky :D

I think my main concern with this is that it replaces nested alternates anywhere in the glob -- even in the part that the user provided.

What I understand is that the problem is mainly about {{ and }} in the base path up to the project. Could we escape the {{ and }} in the project's base-path only instead of applying this fix to the entire glob?

That's true, although the nested alternates will always cause an error even if the user provided them. I thought it might be nice to allow them to avoid manual escaping in that case, but at least they can control that directly, unlike the base paths.

We could also add the escapes to the whole path just before creating the glob. I think that version would also look less hacky than catching this error and retrying, but then we also do the work of trying to replace each time. That was the main reason I tried to keep the replacement in the error case, but we're still only compiling the globs once, so it's probably not a big deal.

That's true, although the nested alternates will always cause an error even if the user provided them. I thought it might be nice to allow them to avoid manual escaping in that case, but at least they can control that directly, unlike the base paths.

The problem I see with this is that they may have wanted to use nested alternates and are now surprised that the glob doesn't match anything.

from https://github.com/astral-sh/ruff/blob/3e840ca839e40d875e28ff7f15c94d88c4d6e7ae/crates/red_knot/tests/cli.rs#L863-L865

ntBre · 2025-02-27T15:05:25Z

Just pushing a test for Windows, I haven't addressed the hacky path stuff yet.

ntBre · 2025-02-27T15:14:34Z

It looks like the regex escapes in tempdir_filter are causing problems, but it works for all of the other tests using this...

C:\/Users\/RUNNER~1\/AppData\/Local\/Temp\/.tmpCazbkF\\{{cookiecutter.repo_name}}\/tests\\*

OH, maybe it's the cookiecutter causing problems again. The escapes for the braces might be getting in the way of the tempdir replacement. Let me try nesting it one level deeper.

MichaReiser · 2025-02-27T15:15:13Z

You can try to use regex::escape to avoid regex problems

ntBre · 2025-02-27T15:19:39Z

tempdir_filter uses regex::escape already, unless I'm misunderstanding. My current guess is that the escapes added by regex::escape are disrupting the TMP replacement because they end up right next to the tempdir.

C:\/Users\/RUNNER~1\/AppData\/Local\/Temp\/.tmpCazbkF\\{{cookiecutter.repo_name}}\/tests\\*
                                                     ^^^ this looks suspicious to me

BurntSushi

Thanks for taking a look at this!

I think the thing I'm not understanding here is why nested alternates are the only problem here? Isn't the problem that we're using arbitrary input to build a glob? And shouldn't that imply the input (in this case, a file path) should always be escaped? Like, the nested alternates is just one way for this problem to manifest. But what if a file path had a * or a [ in it? We would want to escape it there too.

So I guess, can we fix this by just always escaping the literal inputs to a glob?

BurntSushi · 2025-02-27T16:13:59Z

+pub fn try_glob_new(s: &str) -> Result<Glob> {
+    match Glob::new(s) {
+        Err(e) if *e.kind() == globset::ErrorKind::NestedAlternates => {
+            let new = s.replace("{{", "[{][{]").replace("}}", "[}][}]");


This is also not technically right in all cases I think, e.g., [{{], although that's pretty pathological.

ntBre · 2025-02-27T16:42:30Z

Thanks for the review! I failed to notice the globset::escape function. I think we can just use that on the base part of the path that we're adding to the glob pattern. That will be much more robust than my manual escaping here and also leave the user's glob alone in case that's what they intended.

This reverts commit 35cd606.

see BurntSushi/ripgrep#3003

This reverts commit 1f778f0.

This reverts commit 931ff96.

as I saw in one of the early commits, these are a bit redundant with the context provided [here](https://github.com/astral-sh/ruff/blob/764aa0e6a1b16284c1c939332c19d742dae8aee1/crates/ruff_workspace/src/configuration.rs#L179-L182), but I think it's good to have for other uses

ntBre · 2025-02-27T19:48:08Z

This is now a much simpler fix, thanks again for the reviews and suggestions! I also removed the warning, which was the source of the path in the snapshot, ~~so hopefully the Windows tests will pass too~~.

I realized it was also possible to trigger this for the non-absolute path, although in a pretty niche case. The test I added extends per-file-ignores with the working directory set to tempdir/{{cookiecutter.repo_name}}. I peeked at the implementation of Path::absolutize and it just calls Path::absolutize_from with CWD (via the private get_cwd! macro), so I think the change here should be safe.

ntBre · 2025-02-27T20:05:41Z

I was checking for the other uses of Glob::new, and I think this could also be a problem for the FilePath::User variant.

ruff/crates/ruff_linter/src/settings/types.rs

Lines 162 to 166 in cf83584

    
           #[derive(Debug, Clone, CacheKey, PartialEq, PartialOrd, Eq, Ord)] 
        
           pub enum FilePattern { 
        
               Builtin(&'static str), 
        
               User(String, PathBuf), 
        
           }

I'll start adding the escape calls for it too.

MichaReiser

Nice, this looks great

MichaReiser · 2025-02-28T08:01:22Z

-        Ok(Self::User(pattern, absolute))
+        Ok(Self::User(
+            s.to_string(),
+            GlobPath::normalize(s, &*path_dedot::CWD),


Can we introduce a cwd helper function in fs. WASM doesn't like path_dedot::CWD

ruff/crates/ruff_linter/src/fs.rs

Lines 36 to 48 in e7a6c19

pub fn relativize_path<P: AsRef<Path>>(path: P) -> String {

let path = path.as_ref();

#[cfg(target_arch = "wasm32")]

let cwd = Path::new(".");

#[cfg(not(target_arch = "wasm32"))]

let cwd = path_absolutize::path_dedot::CWD.as_path();

if let Ok(path) = path.strip_prefix(cwd) {

return format!("{}", path.display());

}

format!("{}", path.display())

}

Oh good idea, I factored this out into a get_cwd function and call it everywhere in the linter. I left the calls in the ruff and ruff_workspace crates alone for now. Should I replace those too?

MichaReiser · 2025-02-28T08:02:01Z

-            None => fs::normalize_path(path),
-        };
+
+        let project_root = project_root.unwrap_or(&path_dedot::CWD);


Same here, let's avoid spreading path_dedot::CWD over our code base and instead use a helper to maximize WASM compatibility

BurntSushi

LGTM

ntBre added 7 commits February 26, 2025 16:15

reproduce issue

d64470d

fix by checking error

ef04866

test that globbing still works

87878d8

use try_glob_new everywhere Glob::new was used

35cd606

also test in the formatter

0203e68

reindent tests, at least partially

7c86490

show new glob in warning

11917c4

ntBre added the bug An issue describing something that isn't working, or a PR that fixes a bug label Feb 26, 2025

ntBre force-pushed the brent/globbing branch from 349a56f to 11917c4 Compare February 26, 2025 23:03

MichaReiser requested a review from BurntSushi February 27, 2025 07:56

MichaReiser reviewed Feb 27, 2025

View reviewed changes

try red-knot escapes for windows

931ff96

from https://github.com/astral-sh/ruff/blob/3e840ca839e40d875e28ff7f15c94d88c4d6e7ae/crates/red_knot/tests/cli.rs#L863-L865

move cookiecutter away from tempdir

1f778f0

BurntSushi reviewed Feb 27, 2025

View reviewed changes

ntBre added 8 commits February 27, 2025 12:16

Revert "use try_glob_new everywhere Glob::new was used"

5407b9a

This reverts commit 35cd606.

escape glob patterns in project_root

531f45a

bump globset for curly brace escapes

be70948

see BurntSushi/ripgrep#3003

Revert "move cookiecutter away from tempdir"

f119d73

This reverts commit 1f778f0.

Revert "try red-knot escapes for windows"

5e4e237

This reverts commit 931ff96.

accept new snaps

d9521d2

clean up filters, no longer needed without the warning

ef79470

ntBre added 3 commits February 27, 2025 14:09

add failing test case for non-absolute path

7af6c56

pass test and update snap

bd713db

tidy comment

9f4a416

ntBre added 2 commits February 27, 2025 14:59

bring back windows path escapes

55a4772

add a newline to pass format test and avoid path separators

d0eb136

ntBre added 2 commits February 27, 2025 16:18

escape other absolutized paths that will be passed to Glob::new

808c51f

factor out GlobPath type and use for FilePattern::User too

527ddf0

MichaReiser approved these changes Feb 28, 2025

View reviewed changes

MichaReiser requested a review from BurntSushi February 28, 2025 08:02

BurntSushi approved these changes Feb 28, 2025

View reviewed changes

ntBre added 4 commits February 28, 2025 11:22

add fs::get_cwd helper

aafb915

use fs::get_cwd

56c3c14

use get_cwd throughout linter

e5eab84

Merge branch 'main' into brent/globbing

b62a0db

ntBre merged commit d93ed29 into main Mar 3, 2025

ntBre deleted the brent/globbing branch March 3, 2025 14:30

BrewTestBot mentioned this pull request Mar 7, 2025

ruff 0.9.10 Homebrew/homebrew-core#210160

Merged

ntBre mentioned this pull request Mar 27, 2025

[Panic] Server crashes when setting exclude in VS Code settings #17010

Closed

	pub fn relativize_path<P: AsRef<Path>>(path: P) -> String {
	let path = path.as_ref();

	#[cfg(target_arch = "wasm32")]
	let cwd = Path::new(".");
	#[cfg(not(target_arch = "wasm32"))]
	let cwd = path_absolutize::path_dedot::CWD.as_path();

	if let Ok(path) = path.strip_prefix(cwd) {
	return format!("{}", path.display());
	}
	format!("{}", path.display())
	}

Conversation

ntBre commented Feb 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Test Plan

Uh oh!

github-actions Bot commented Feb 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

ruff-ecosystem results

Linter (stable)

Linter (preview)

Formatter (stable)

Formatter (preview)

Uh oh!

ntBre commented Feb 26, 2025

Uh oh!

MichaReiser commented Feb 27, 2025

Uh oh!

MichaReiser Feb 27, 2025

Choose a reason for hiding this comment

Uh oh!

ntBre Feb 27, 2025

Choose a reason for hiding this comment

Uh oh!

MichaReiser Feb 27, 2025

Choose a reason for hiding this comment

Uh oh!

ntBre commented Feb 27, 2025

Uh oh!

ntBre commented Feb 27, 2025

Uh oh!

MichaReiser commented Feb 27, 2025

Uh oh!

ntBre commented Feb 27, 2025

Uh oh!

BurntSushi left a comment

Choose a reason for hiding this comment

Uh oh!

BurntSushi Feb 27, 2025

Choose a reason for hiding this comment

Uh oh!

ntBre commented Feb 27, 2025

Uh oh!

ntBre commented Feb 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ntBre commented Feb 27, 2025

Uh oh!

MichaReiser left a comment

Choose a reason for hiding this comment

Uh oh!

MichaReiser Feb 28, 2025

Choose a reason for hiding this comment

Uh oh!

ntBre Feb 28, 2025

Choose a reason for hiding this comment

Uh oh!

MichaReiser Feb 28, 2025

Choose a reason for hiding this comment

Uh oh!

BurntSushi left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

ntBre commented Feb 26, 2025 •

edited

Loading

github-actions Bot commented Feb 26, 2025 •

edited

Loading

`ruff-ecosystem` results

ntBre commented Feb 27, 2025 •

edited

Loading