Gilnaa / globwalk

MIT License
47 stars 14 forks source link

Globs that start with "./" broken #28

Open nabijaczleweli opened 4 years ago

nabijaczleweli commented 4 years ago

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 commented 4 years ago

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 commented 3 years ago

@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 commented 3 years ago

Appears so, yes.

Gilnaa commented 3 years ago

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:

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 commented 3 years ago

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 commented 3 years ago

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): https://github.com/rust-lang/rust/issues/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 commented 3 years ago

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.