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

Globs that start with "./" broken #28

Open
nabijaczleweli opened this issue Jun 5, 2020 · 7 comments
Open

Globs that start with "./" broken #28

nabijaczleweli opened this issue Jun 5, 2020 · 7 comments

Comments

@nabijaczleweli
Copy link

nabijaczleweli commented Jun 5, 2020

Consider the following main.rs:

extern crate globwalk;
fn main() {
    println!("dot");
    for f in globwalk::glob("./globwalk/**/*.rs").unwrap() {
        println!("  {:?}", f);
    }

    println!("nodot");
    for f in globwalk::glob("globwalk/**/*.rs").unwrap() {
        println!("  {:?}", f);
    }
}

And Cargo.toml:

[package]
name = "glest"
version = "0.1.0"

[dependencies]
globwalk = "0.8.0"

Running the resulting binary in a directory containing a checkout of globwalk:

T:\glest>target\debug\glest
dot
nodot
  Ok(DirEntry(".\\globwalk\\examples\\list.rs"))
  Ok(DirEntry(".\\globwalk\\src\\doctests.rs"))
  Ok(DirEntry(".\\globwalk\\src\\lib.rs"))
  Ok(DirEntry(".\\globwalk\\tests\\docs.rs"))

Note how an otherwise identical glob which started with ./ returned no results.

@Gilnaa
Copy link
Owner

Gilnaa commented Jun 6, 2020

While looking into this I also found out that ignore's * doesn't descend into subdirs, but *.* does.

I apologize, but I'm not sure that I will have time to fix this. (but of course PRs are welcome)

You might have more luck using ignore::WalkBuilder with WalkBuilder::overrides.

@FibreFoX
Copy link

@nabijaczleweli can you confirm, that it is not only ./ but ../ too? While learning Tera https://github.com/Keats/tera, which uses globwalk, I found this issue too.

I am used to put things at some different location, which makes me require to use relative paths on an upper level.

@nabijaczleweli
Copy link
Author

Appears so, yes.

@Gilnaa
Copy link
Owner

Gilnaa commented Dec 8, 2020

Hey guys,

Also tagging @mitsuhiko and @Keats , because your crates depend on globwalk.
(and @killercup , which triggered this crate years ago)

Sorry for taking such long time to address this.
I would really like to solve this issue, but there are several problems with the fundamental way globwalk works.
Nothing that can't be solved, but this is a discussion about defaults, which I'm infamously bad at.

Currently, globwalk searches for matches recursively, meaning that *.rs is equivalent to **/*.rs and finds foo.rs as well as foo/mod.rs.
This is very much unlike how actual globs work in the wild, which can be good or bad, depending on your point of view.
This does differentiate this crate from glob, which was dormant at the time of the writing of globwalk, but has since been revived and adopted by rust-lang-nursery. (though I'm not sure if any of the original warts have been fixed).

Also note that absolute paths do not work intuitively at all:

$ tree a
a
└── b
    ├── c
    │   └── foo.rs
    └── d
        └── foo.rs
$ cargo run --example list '/a'
"./a"

This can be done in several ways and I'd like to hear your opinions:

  1. Deprecate this crate and refer to glob or some other probably-now-existing-crate written by @BurntSushi
  2. Detect explicitly absolute and relative paths:
  • Strip ./ and ignore it
  • Error on ../ and absolute paths patterns
  1. Detect explicitly absolute and relative paths:
  • Disable recursive lookup when either is encountered. (or hide it behind some flag)

Almost everything is a slight breaking change, but since this crate is <1.0.0 and these use cases didn't work before, I don't worry about it that much.

Thoughts?

@Gilnaa
Copy link
Owner

Gilnaa commented Dec 8, 2020

Also note that since I'm not doing anything fancy with the pattern itself, all kinds of exotic patterns are not supported as well:

➜  globwalk git:(master) ✗ tree a
a
└── b
    ├── c
    │   └── foo
    └── foo
➜  globwalk git:(master) ✗ cargo run -q --example list 'a/b/foo' 2>/dev/null
"./a/b/foo"
➜  globwalk git:(master) ✗ cargo run --example list 'a/b/foo' 2>/dev/null
"./a/b/foo"
➜  globwalk git:(master) ✗ cargo run --example list 'a/b/c/foo' 2>/dev/null
"./a/b/c/foo"
➜  globwalk git:(master) ✗ cargo run --example list 'a/b/c/../foo' 2>/dev/null
➜  globwalk git:(master) ✗ cargo run --example list 'a/b/c/./foo' 2>/dev/null
➜  globwalk git:(master) ✗

@FibreFoX
Copy link

FibreFoX commented Dec 9, 2020

Thanks for the in-depth response @Gilnaa 😺

Personally I would prefer "proper" resolving of the given path, similar like bash works. Working with absolute paths even becomes evil on Windows, which is a bug in Rust itself (or at least it seems to me): rust-lang/rust#42869

I am using globwalk indirectly on my local Windows machine and on a RaspberryPi by using the Tera-lib (https://github.com/Keats/tera), and I would ❤️ to provide a proper fix for this. I suppose you are open to any pull-request on this matter? Maybe I can find some time to tackle this issue, might even get me more into the Rust-ecosystem.

@Gilnaa In regards to compatibility ... would you prefer to have an option on this new behavior, or would you prefer a breaking change?

@Gilnaa
Copy link
Owner

Gilnaa commented Dec 9, 2020

Still not sure what behaviour I want. I lean towards having the default non recursive (like you'd expect).
I think the best course of action will be to stop using ignore (which was originally a bit of a hack) and either use globset instead, or just deprecate this crate and augment globset. (it does have this as an optional goal)

Always open to PRs of course, but the majority of the work seems to be design for now.

eguiraud added a commit to eguiraud/tera that referenced this issue Jan 24, 2023
This is because globwalk always returns an empty list in that
case, which is likely not what users expect.

The corresponding issue in globwalk is
Gilnaa/globwalk#28.

See Keats#574 for more discussion.
eguiraud added a commit to eguiraud/tera that referenced this issue Jan 26, 2023
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 added a commit to eguiraud/tera that referenced this issue Jan 26, 2023
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.
Keats pushed a commit to Keats/tera that referenced this issue Mar 8, 2023
* Add test for globs starting with "./"

This is a regression test for #574.

* Always canonicalize glob paths passed to Tera

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

This fixes #574.
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

No branches or pull requests

3 participants