Feat globs can be relative for newglob and cli arg

[NickHackman]

Relative paths now work for both the command line argument and newglob

Example:

Starting in directory ~/Code/riv you can run $ riv "../../Pictures/**/*" which will display all pictures in ~/Pictures

:ng ~/Pictures then :ng copied_images then back again :ng ..

Paths are now normalized removing "."s and ".."s

Example:

$ riv "~/Pictures/././././../Pictures/" results in ~/Pictures

Removal of escaped spaces is now ONLY done for Unix and not for Windows

using cfg!(unix)

[gurgalex]

@NickHackman @Davejkane

Relative paths do not work on Windows.

I'll get some screenshots shortly.

[NickHackman]

@gurgalex okay sweet, I wonder what's going wrong on that end. :thinking:

[gurgalex]

So, launching riv from the linux equivelant ~ in windows With the following folder contents.

C:\Users\jamie>dir *.png
 Volume in drive C has no label.
 Volume Serial Number is E68F-1D24

 Directory of C:\Users\jamie

05/27/2019  02:30 PM            85,848 rust-logo-512x512.png
               1 File(s)         85,848 bytes
               0 Dir(s)   5,101,404,160 bytes free

riv finds no files when launched with no arguments riv

With ng . the glob has a strange prefix before the C drive. It also is omitting the jamie directory under Users (See screenshot)

[gurgalex]

With ng ~ the strange prefix is there, but the rest of the glob is correct (includes jamie/*.png)

[gurgalex]

I can confirm that the development branch resolves ng ~ correctly and finds the one picture.

[NickHackman]

@gurgalex @Davejkane hmm, I have no idea what would be causing that. I'll setup a windows VM and I'll try testing things out, I'll get back to you guys later tonight on the state of this.

Edit: Working on this more tomorrow

[gurgalex]

@NickHackman Seems path normalization is a desired feature for the stdlib as well https://github.com/rust-lang/rfcs/issues/2208

"some" test cases for Windows though... (search for tc! https://github.com/rust-lang/rust/pull/47363/files

[NickHackman]

@Davejkane Sorry that took so long, I've had other stuff come up (I'm in summer semester and in a project class).

@gurgalex I tried really hard to use std::fs::canonicalize the reason it's not being used is because it requires the path to exist, and in our case that's awful. All globs fail, in the implementation I had at the start of this PR it would be a greater time complexity. Since it would call canonicalize, if it failed then it removed the glob then it called it again. This implementation follows symlinks like canonicalize, but doesn't require the path to exist which makes our lives easier and requires just a linear pass through to normalize.

[gurgalex]

@NickHackman Not to worry about not using the std lib.

Small issue with path escaping on Linux (Windows testing soon)

With the directory ~/Down\ loads The escaped path should be ~/Down\\\ loads ng and df both interpret that escaped string as ~/Down\\ loads

[NickHackman]

Okay I see, that is a rough edge case...

So the solution would be to replace all escaped things with the thing that's being escaped.

I'll get back to you shortly on that! Maybe a regex replace would be able to do that.

Thanks @gurgalex that's a really good test case I appreciate it

edit: formatting

[NickHackman]

Okay cool, @gurgalex that now works as expected, thanks again. It also works for stuff like ~/Test\'\', ~/Test\"\", ~/a\ b\ c\ d, etc

Anything wrong on the Windows side? There shouldn't be afaik.

Edit: Disregard me below being really bad at merging

[gurgalex]

@NickHackman If test cases could be put in for path normalization. That would be great if the implementation needs to change later.

It seems like you had at least one earlier?

[NickHackman]

@gurgalex I think we need test cases for everything, I've recently switched to a TDD mindset, but it's really hard to test things at the OS level.

I think we should follow Exa that uses Vagrant

Furthermore, are we doing just public facing method testing or are we doing private methods as well? @Davejkane

Edit: Yeah good, idea

[gurgalex]

I agree, I at least am testing things that don't rely on the OS in the refactor and implementation of #48 https://github.com/gurgalex/riv/blob/4cc01237db2ad270acb61ed88552f3ea04671c53/src/paths.rs#L292

[gurgalex]

Edited to add glob pattern path

@NickHackman

You could test the output of the path returned.

Example. This works on linux.

use std::path::PathBuf;
fn main() {
    let path = PathBuf::from(r"C:\windows\system32/*.dll");
    dbg!(&path);
    assert_eq!(path, PathBuf::from(r"C:\windows\system32/*.dll"));
}

[gurgalex]

Yeah, globs would be harder. I guess making sure the glob string gotten from the user converts to the glob you'd pass to glob is fine. I assume that glob::glob already works.

[NickHackman]

Yeah, oddly enough... This test case fails when running on Unix

#[test]
fn normalize_path_prefix_windows() {
    let path = PathBuf::from(r"C:\windows\.");
    assert_eq!(normalize_path(path), PathBuf::from(r"C:\windows"));
}

Which is really weird, since the Unix alternative passes? Also pretty sure this functions on windows as well...

[Davejkane]

@NickHackman & @gurgalex, I'd definitely be interested in getting some more testing in this project. It kinda just started as way to scroll through some pictures. But it's definitely time for a bit more professionalism now that it's coming together as a piece of software.

@NickHackman What's the status of this branch? Are you happy with it now, or are there still unresolved things?

[NickHackman]

@Davejkane I'm happy with the state of this branch. There isn't anything unresolved :smiley:

As we discussed above I have Unix test cases if you want (since all Windows test cases will fail on a Unix machine and vice versa) if you'd like those I can push those onto this PR, but they will fail on the opposite platform... as a result of how Rust Component views Rootdir differs based on platform. I'd eventually like to write test cases for these methods, but we need to set up some sort of setup for that, which I'll start looking into.

[NickHackman]

@Davejkane I'm still new to this, so maybe I'm just confused, but don't I need write access?

[Davejkane]

@NickHackman yeah, you're right. I'll do the merge.