Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Always canonicalize glob paths passed to Tera #799

Merged
merged 2 commits into from
Mar 8, 2023

Conversation

eguiraud
Copy link
Contributor

This is to work around an issue in globwalk (Gilnaa/globwalk#28) due to which paths starting with ./ or ../ never match anything. A regression test has been added as well.

This fixes #574.

This is a regression test for Keats#574.
@eguiraud
Copy link
Contributor Author

The CI complains that let...else statements are unstable citing rust-lang/rust#87335, but as far as I can tell they have been stabilized already.

@eguiraud
Copy link
Contributor Author

Ah it's because the CI still uses rustc version 1.56.0. I can change the let...else to a match statement if using such an old version of rustc (from October '21, apparently) is intentional.

@Keats
Copy link
Owner

Keats commented Jan 26, 2023

Yep, no need to bump the MSRV for such a small change.

I remember some issues with canonicalizing paths on Windows but my memory is fuzzy :/

This is to work around an issue in globwalk
(Gilnaa/globwalk#28) due to which paths
starting with `./` or `../` never match anything.

This fixes Keats#574.
@eguiraud eguiraud force-pushed the always-canonicalize-globs branch from 27e5a28 to 7bb748a Compare January 26, 2023 19:07
@eguiraud
Copy link
Contributor Author

Changed the let...else to a match 👍

@eguiraud
Copy link
Contributor Author

eguiraud commented Mar 8, 2023

Hi, is this ready to merge? 👀

@Keats
Copy link
Owner

Keats commented Mar 8, 2023

Yes, i just needed to block some time to merge all the PR and do a release

@Keats Keats merged commit 9479c28 into Keats:master Mar 8, 2023
vimpostor added a commit to vimpostor/tera that referenced this pull request Apr 6, 2023
Regression was introduced in 9479c28.

Since that commit providing an invalid glob to tera will make it fail to
create an instance, first (in that commit) by making it panic and since
1f95878 by making it return an error.

While returning an error is not entirely unlogical, it doesn't match
what most languages do with invalid globs.

- Bash will by default return an empty set on invalid globs, as the
  `failglob` option is off by default
- Python will likewise return the empty set instead of throwing an
  exception, when doing something like `glob.glob("/dev/null/*")`
- Rust's `globwalk` will also not error, but return an empty set

In fact, we use globwalk in tera and the only reason we panic is by
accident, because `std::fs::canonicalize()` checks the path.

It is better to match other language's glob behaviour, therefore we
resort back to the original path if `canonicalize()` fails to return the
empty set again when encountering an invalid glob.

Even more so, since this caused a lot of regressions already, including
in zola.

Fixes getzola/zola#2150

ref: Keats#819
ref: Keats#820
ref: Keats#799
vimpostor added a commit to vimpostor/tera that referenced this pull request Apr 6, 2023
Regression was introduced in 9479c28.

Since that commit, providing an invalid glob to tera will make it fail
to create an instance, first (in that commit) by making it panic and
since 1f95878 by making it return an
error.

While returning an error is not entirely unlogical, it doesn't match
what most languages do with invalid globs.

- Bash will by default return an empty set on invalid globs, as the
  `failglob` option is off by default
- Python will likewise return the empty set instead of throwing an
  exception, when doing something like `glob.glob("/dev/null/*")`
- Rust's `globwalk` will also not error, but return an empty set

In fact, we use globwalk in tera and the only reason we panic is by
accident, because `std::fs::canonicalize()` checks the path.

It is better to match other language's glob behaviour, therefore we
resort back to the original path if `canonicalize()` fails to return the
empty set again when encountering an invalid glob.

We should especially restore the previous behaviour, as this caused a
lot of regressions already, including in zola.

Fixes getzola/zola#2150

ref: Keats#819
ref: Keats#820
ref: Keats#799
vimpostor added a commit to vimpostor/tera that referenced this pull request Apr 6, 2023
Regression was introduced in 9479c28.

Since that commit, providing an invalid glob to tera will make it fail
to create an instance, first (in that commit) by making it panic and
since 1f95878 by making it return an
error.

While returning an error is not entirely unlogical, it doesn't match
what most languages do with invalid globs.

- Bash will by default return an empty set on invalid globs, as the
  `failglob` option is off by default
- Python will likewise return the empty set instead of throwing an
  exception, when doing something like `glob.glob("/dev/null/*")`
- Rust's `globwalk` will also not error, but return an empty set

In fact, we use globwalk in tera and the only reason we panic is by
accident, because `std::fs::canonicalize()` checks the path.

It is better to match other language's glob behaviour, therefore we
resort back to the original path if `canonicalize()` fails.

We should especially restore the previous behaviour, as this caused a
lot of regressions already, including in zola.

Fixes getzola/zola#2150

ref: Keats#819
ref: Keats#820
ref: Keats#799
vimpostor added a commit to vimpostor/tera that referenced this pull request Apr 10, 2023
Regression was introduced in 9479c28.

Since that commit, providing an invalid glob to tera will make it fail
to create an instance, first (in that commit) by making it panic and
since 1f95878 by making it return an
error.

While returning an error is not entirely unlogical, it doesn't match
what most languages do with invalid globs.

- Bash will by default return an empty set on invalid globs, as the
  `failglob` option is off by default
- Python will likewise return the empty set instead of throwing an
  exception, when doing something like `glob.glob("/dev/null/*")`
- Rust's `globwalk` will also not error, but return an empty set

In fact, we use globwalk in tera and the only reason we panic is by
accident, because `std::fs::canonicalize()` checks the path.

It is better to match other language's glob behaviour, therefore we
resort back to the original path if `canonicalize()` fails.

We should especially restore the previous behaviour, as this caused a
lot of regressions already, including in zola.

Fixes getzola/zola#2150

ref: Keats#819
ref: Keats#820
ref: Keats#799
Keats pushed a commit that referenced this pull request Apr 16, 2023
Regression was introduced in 9479c28.

Since that commit, providing an invalid glob to tera will make it fail
to create an instance, first (in that commit) by making it panic and
since 1f95878 by making it return an
error.

While returning an error is not entirely unlogical, it doesn't match
what most languages do with invalid globs.

- Bash will by default return an empty set on invalid globs, as the
  `failglob` option is off by default
- Python will likewise return the empty set instead of throwing an
  exception, when doing something like `glob.glob("/dev/null/*")`
- Rust's `globwalk` will also not error, but return an empty set

In fact, we use globwalk in tera and the only reason we panic is by
accident, because `std::fs::canonicalize()` checks the path.

It is better to match other language's glob behaviour, therefore we
resort back to the original path if `canonicalize()` fails.

We should especially restore the previous behaviour, as this caused a
lot of regressions already, including in zola.

Fixes getzola/zola#2150

ref: #819
ref: #820
ref: #799
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

globwalk does not work with several relative paths
2 participants